-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create spright-chat-conversation and spright-chat-message components #2529
base: main
Are you sure you want to change the base?
Conversation
packages/storybook/src/spright/chat/chat-message-matrix.stories.ts
Outdated
Show resolved
Hide resolved
packages/storybook/src/spright/chat/chat-message-matrix.stories.ts
Outdated
Show resolved
Hide resolved
packages/storybook/src/spright/chat/chat-message-matrix.stories.ts
Outdated
Show resolved
Hide resolved
packages/storybook/src/spright/chat/chat-message-matrix.stories.ts
Outdated
Show resolved
Hide resolved
@rajsite this is getting close to having all the feedback resolved so I'm moving it to Ready so you can take a look when you have time. |
packages/storybook/src/spright/chat/chat-message-matrix.stories.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not finished reviewing but this is some initial feedback. Providing it sooner so it can be addressed sooner but as discussed I plan to finish review early next week.
])} | ||
`); | ||
|
||
export const extraWideMessageSizing: StoryFn = createStory(html` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing this case should cover is large message values that have a fixed size / are not resizeable.
What II want to see if if we should render a horizontal scrollbar in those cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added story messageWithFixedSizeSizing
. The message has a rich text viewer component that it is a fixed width. This causes a scrollbar to show for the text. Adding a fixed size to the chat message does not show a scrollbar. The text goes beyond the size of the chat message. Is this okay?
If we want the actual chat message to show a scrollbar, I will need help with the styling. I tried adding overflow-y: scroll;
to the host section of the chat message style and it did not work. Also, I am not sure if that is what we want. The content of the message should decide how big it should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree the overflowing text means it doesn't look great right now
But I think the fix might be as simple as setting overflow-x: scroll
on the div
inside the chat-message
(x means horizontal scrollbar). The behavior I think we want is for messages to grow vertically (no max-height
, no overflow-y
) and to be restricted horizontally with a horizontal scrollbar (max-width
, overflow-x: scroll
). Here's what that looks like when I make this change in dev tools (macOS)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another weird behavior I noticed is that the green message background color seems to change slightly as the window resizes. I have no idea what would cause this; we can defer fixing it until later if needed.
Screen.Recording.2025-02-11.at.4.23.13.PM.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jattasNI @joseahdz pushed some changes to the matrix stories that covers more cases of content with different sizes inside of viewports of different sizes more completely (and without coupling to complex components like rich-text).
It definitely shows some areas of improvements but we could decide how to approach that (it's just helpful having the coverage now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi re-read jesse's proposal and applied the scrollbar to the inner message content, just pushed that change
@@ -0,0 +1,7 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of the changes I pushed:
- Keeping the storybook file structure and naming aligned with the components. So story naming aligns to
chat-conversation
- Made the storybook templates more consistent (use double quotes on attributes, use Enum references instead of hard coded strings)
- Followed the pattern of mapping column and made the chat-message stories internal. Users are expected to use chat-conversation with chat-message and chat-message is not standalone, so just document it inside of chat-conversation
- Removed most of the chat-message standalone matrix stories. We really only care about chat-conversation's containing chat-messages so focusing on those matrix stories.
Pull Request
🤨 Rationale
Some Intelligent Test application teams need to create a chat interface. The chat interface will be shared by all those teams. The chat interface will be built using Blazor and Nimble components. In this PR, I am adding two Nimble components spright-chat-conversation and spright-chat-message components that will be used in the chat interface.
The spec detailing the new components is here #2513.
👩💻 Implementation
🧪 Testing
Added some tests, story and theme matrix
✅ Checklist