Skip to content
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

Added new slots for the spright-chat-message to allow adding action buttons and follow-up prompts #2552

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

joseahdz
Copy link
Contributor

@joseahdz joseahdz commented Mar 12, 2025

Pull Request

🤨 Rationale

UX design calls for adding action buttons to chat messages to allow user to edit prompts and provide feedback on AI responses.
Also, messages can have suggested follow-up button prompts and those should be below the action buttons.

👩‍💻 Implementation

Added slots to allow adding buttons to the left and bottom-left side of a message. In the future, we might allow adding buttons to the top-left, top-left and bottom-right. This is the main reason I have decided to call the bottom slot left-bottom.
I also have added a new slot for follow-up prompts which is below the bottom action buttons slot.

🧪 Testing

Since changes are visual only, I added a new interactions from states matrix.

✅ Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

The action controls provide "actions" that can be done to the chat-message like copying and thumbs up/down.
Action buttons on outbound messages appear on the left side when hovering over the message row
* main:
  applying package updates [skip ci]
  Update to .net 8.0.406 (#2545)
  Changed Rich Text Viewer markdown property to an attribute (#2542)
  applying package updates [skip ci]
  Blazor chat components (#2541)
* main:
  Created Blazor wrapper for nimble-rich-text-viewer component (#2544)
* main:
  Add usage guidance about table column mapping performance (#2548)
  Improve usage guidance for select filter mode (#2549)
  applying package updates [skip ci]
  Update npm dependencies update to latest (#2547)
  Update npm dependencies lock update only (#2546)
  applying package updates [skip ci]
* main:
  applying package updates [skip ci]
  Migrate to `@ni/fast-*` packages (#2550)
Copy link
Contributor

@jattasNI jattasNI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looked at the spec and the docs, not the implementation yet

@joseahdz joseahdz marked this pull request as ready for review March 27, 2025 13:38
@joseahdz joseahdz requested review from rajsite and jattasNI April 1, 2025 02:27
display: none;
}

[part='end'] ::slotted(*) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some changes to make to this styling. We want the element which contains the content to be sized according to the content but right now it's tiny:
image

And while we may want the slot to provide some initial spacing, that should largely be applied to the container, not set on the buttons that the client provides.

image

And finally we should check if there are tokens we can use rather than hard coding values like 16px.

I can push some changes to address this if you'd like.

Copy link
Contributor Author

@joseahdz joseahdz Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will greatly appreciate if you make those changes. Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I'll get to this today but tomorrow morning looks open.

@@ -119,6 +144,15 @@ export const systemSizing: StoryFn = createStory(html`
])}
`);

export const messageInteractionsThemeMatrix: StoryFn = createMatrixThemeStory(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good that you're adding matrix test coverage for the new slots. But there are a few issues:

  1. it's not necessary to include coverage for states combinations like hover and active. Those states will be covered by existing button tests and we don't need to verify that they still work inside a message slot
  2. we should add coverage for layout and sizing. If there are multiple buttons, what's the default padding between them? If there are more buttons than the width of the message, how do they layout? If there are more buttons than fit in the width of the conversation, how do they layout?

For 2 I think the designers are still deciding but we could add the tests with whatever behavior we get today and then when we adopt the designers' suggestions we'll see a useful diff in the snapshots for the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there examples I can look at for those types of tests? If not, I am concern about how much time it will take to write them. I am under a very tight schedule and I need to minimize the amount of time I spend on non-chat Blazor UI changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your existing conversation sizing tests cover some similar cases. You could just add a few more cases to those:

  1. a couple footer action buttons and end buttons on the same message
  2. lots of footer action buttons
    • wider than the message but narrower than the conversation
    • wider than the conversation
  3. lots of end buttons
    • wider than the message but narrower than the conversation
    • wider than the conversation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Milan and I discussed a couple more ideas. Let me take a stab at this along with the styling changes. I think we want some of this in a new matrix test and some of it in the existing sizing tests.

* SprightChatMessage configuration options
* @public
*/
export type ChatMessageOptions = FoundationElementDefinition & StartEndOptions;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with the our/FAST's StartEndOptions pattern. It looks like this follows what the tab does by exporting this type and using it from the template. But I was expecting to see the pattern in more other components. @rajsite could you double check that it's being applied correctly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants