Separate components for spright chat message types #2835
Conversation
jattasNI
changed the title
jattasNI
changed the title
| @@ -0,0 +1,53 @@ | |||
| import { observable } from '@ni/fast-element'; | |||
There was a problem hiding this comment.
I'm open to feedback about file layout. I went with this for spright-components and spright-angular:
chat/message // deprectated
chat/message/inbound
chat/message/outbound
chat/message/system
This roughly matches what we do for table columns although we decided not to put them under the /table path.
table-column/anchor
table-column/date-text
...
I decided to keep these under the chat path because all of our other chat components are there:
chat/input
chat/conversation
| @@ -0,0 +1,45 @@ | |||
| import { css } from '@ni/fast-element'; | |||
There was a problem hiding this comment.
I debated trying to share more styles but opted not to. There is some duplication right now (layout, fonts) but also some uniqueness (slots for inbound messages, bubble background for outbound messages, different justify-content settings to control left/center/right layout).
Ultimately I think these are more likely to diverge than converge over time and the benefit of consistency didn't seem to outweigh the mental overhead of reasoning about custom styles applied on top of shared styles. I'd propose we start here and revisit as we add more messages.
| namespace SprightBlazor; | ||
|
|
||
| /// <summary> | ||
| /// SprightChatMessage is deprecated. Use specific message component types instead. |
There was a problem hiding this comment.
I tried using the [Obsolete] tag but as Milan / Darren predicted, that caused build failures (even within Nimble, presumably also within clients) because warnings are treated as errors.
| export const chatMessageRichText: StoryObj<ChatMessageRichTextArgs> = { | ||
| render: createUserSelectedThemeStory(html` | ||
| <${chatMessageTag} message-type="${x => ChatMessageType[x.messageType]}"> | ||
| <${chatMessageInboundTag}> |
There was a problem hiding this comment.
I changed this from outbound to inbound to more closely match real product use cases for rich text messages. This explains the one Chromatic diff that isn't a new story.
jattasNI
requested review from
mollykreis,
msmithNI and
rajsite
as code owners
| min-width: 16px; | ||
| min-height: 16px; |
There was a problem hiding this comment.
Is there a reason not to use a token value for the min width/height styling?
Same in the outbound message styles.
There was a problem hiding this comment.
Just laziness / perpetuating the pattern from the original implementation! We have a lot of tokens with the value of 16px but none of them had quite the correct naming for this purpose. But given how many places this is used I agree a token makes sense. I'm on the fence between using standardPadding or creating a new token. Feel free to weigh in if you have an opinion, or I'll just pick one.
There was a problem hiding this comment.
If there isn't a better option standard padding is fine. It's the generic size ramp with an unhelpful name at this point (essentially the base tokens for size) and we don't have super well defined high level sizing tokens
There was a problem hiding this comment.
Oh, we actually document it as that lol https://nimble.ni.dev/storybook/index.html?path=/docs/tokens-size-ramp--docs
Pull Request
🤨 Rationale
One of the tasks in #2610 is to create separate components for different message types instead of a single chat message with a
message-typeattribute. This would have some benefits:footer-actionsandendare only available on inbound messages) but are currently present on all👩💻 Implementation
spright-chat-message-inbound,spright-chat-message-outbound,spright-chat-message-systemby copying existing message and removing code that was specific to other types.spright-chat-messageas deprecated. We can remove it once clients migrate to the new messages.🧪 Testing
✅ Checklist