-
Notifications
You must be signed in to change notification settings - Fork 12
Add slot for toolbar #2857
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
base: main
Are you sure you want to change the base?
Add slot for toolbar #2857
Conversation
packages/angular-workspace/example-client-app/src/app/customapp/customapp.component.html
Outdated
Show resolved
Hide resolved
β¦n/toolbar-in-conversation
| <spright-chat-conversation> | ||
| <nimble-toolbar slot="toolbar" > | ||
| <nimble-icon-messages-sparkle slot="start"></nimble-icon-messages-sparkle> | ||
| <span class="toolbar-title" i18n>AI Assistant</span> |
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.
This app isn't localized, so there isn't a reason to use i18n tags.
| <span class="toolbar-title" i18n>AI Assistant</span> | |
| <span class="toolbar-title">AI Assistant</span> |
| <nimble-icon-messages-sparkle slot="start"></nimble-icon-messages-sparkle> | ||
| <span class="toolbar-title" i18n>AI Assistant</span> | ||
| <nimble-button appearance="ghost" content-hidden | ||
| i18n-title title="Create new chat" slot="end"> |
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.
| i18n-title title="Create new chat" slot="end"> | |
| title="Create new chat" slot="end"> |
| it('should have a toolbar slot element in the shadow DOM', async () => { | ||
| await connect(); | ||
| const toolbarSlot = element.shadowRoot?.querySelector('slot[name="toolbar"]'); | ||
| expect(toolbarSlot).not.toBeNull(); | ||
| }); |
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.
It seems like there are some missing tests here. Particularly, I think there should be some tests around the hidden styling and how it updates when the toolbar does (or does not) have elements.
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.
@mollykreis for the input slot we're relying on the conversation's matrix tests to cover that behavior by including a state with the slot empty and a state with it populated. Same for the dialog footer being empty, which follows a similar pattern (see #2579 for discussion of the pattern). Since we have a similar matrix test for the toolbar slot, I'd advocate that it's ok to skip a unit test here too. Thoughts?
|
|
||
| #### Toolbar example | ||
|
|
||
| ```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.
A few minor pieces of feedback on this html snippet:
- It should be written in a framework-agnostic way (i.e. don't use
i18ntags) - Everything within the
nimble-toolbarelement looks too far indented - It isn't clear if the "Create new chat" button is supposed to be an example of a text+icon button or an icon-only button. Either way, it isn't quite right:
- the
nimble-icon-pencil-to-rectanglebutton should be in the"start"slot - the button should have text content (i.e. "Create new chat")
- if it is intended to be an icon-only button, it should be marked as
content-hidden
- the
| <${iconMessagesSparkleTag} slot="start"></${iconMessagesSparkleTag}> | ||
| <${buttonTag} appearance="ghost" slot="end" title="Create new chat" content-hidden> | ||
| Create new chat | ||
| <${iconPencilToRectangleTag} slot="start"></${iconPencilToRectangleTag}> | ||
| </${buttonTag}> |
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.
nit: The content within the toolbar looks like it's indented one extra level.
| }, | ||
| toolbar: { | ||
| description: | ||
| 'A slot to optionally include toolbar content which will be displayed on top of the conversation.', |
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.
nit: it'd be good to make it clear that you slot the entire toolbar, not the content of the toolbar
| 'A slot to optionally include toolbar content which will be displayed on top of the conversation.', | |
| 'A slot to optionally include a \`${toolbarTag}\` which will be displayed on top of the conversation.', |
| </style> | ||
| <${chatConversationTag} ${ref('conversationRef')} appearance="${x => x.appearance}"> | ||
| ${when(x => x.toolbar, html<ChatConversationArgs>` | ||
| <${toolbarTag} slot='toolbar' class='toolbar'> |
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'm not seeing anything that requires the class to be present so let's remove it for simplicity. (Let me know if I missed a reason it needs to be there)
| <${toolbarTag} slot='toolbar' class='toolbar'> | |
| <${toolbarTag} slot='toolbar'> |
| `); | ||
|
|
||
| const conversationWithToolbar = ( | ||
| [heightLabel, height]: HeightStates, |
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.
In the rendered matrix story the state called "conversation is shorter than the height of the messages" isn't actually true: because we removed the input control the conversation doesn't show a scrollbar.
Could you adjust the height or the conversation contents to make it true? The goal is for the story to show a scrollbar so that we can verify the right part of the conversation is scrollable. It looks like the behavior is correct (just the messages part scrolls, not the toolbar or input) but we want the test to show that.
|
|
||
| 1. Lays out messages vertically based on their order. | ||
| 1. Displays a vertical scrollbar if there are more messages than fit in the height allocated to the conversation. | ||
| 1. Includes a slot to place toolbar content (such as buttons or menu buttons) on top of the conversation. |
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.
Similar feedback as I left on the story: we should make it clear that the client provides the toolbar and the content, not just the content.
| 1. Includes a slot to place toolbar content (such as buttons or menu buttons) on top of the conversation. | |
| 1. Includes a slot to place a toolbar (and its content such as buttons or menu buttons) on top of the conversation. |
| - chat messages are added to the default slot. The DOM order of the messages controls their screen order within the conversation (earlier DOM order => earlier message => top of the conversation) | ||
| - a single chat input can optionally be added to the `input` slot. It will be placed below the messages. | ||
| - a single chat input can optionally be added to the `input` slot. It will be placed below the messages | ||
| - optional slot for toolbar content (such as buttons or menu buttons) that will be displayed above the messages. This can be used for actions like starting a new conversation, copying all messages, or other conversation-level operations. |
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.
| - optional slot for toolbar content (such as buttons or menu buttons) that will be displayed above the messages. This can be used for actions like starting a new conversation, copying all messages, or other conversation-level operations. | |
| - a toolbar can optionally be added to the `toolbar` slot. The toolbar will be displayed above the messages. The toolbar content can be buttons or menus used for actions like starting a new conversation, copying all messages, or other conversation-level operations. |
| .toolbar { | ||
| display: flex; | ||
| flex-direction: column; |
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 think we could delete flex-direction: column, but I want to check that I'm understanding its purpose.
This flex-direction controls how children of the .toolbar div will be laid out, correct? And we typically expect only one direct child: a single nimble-toolbar? So for our known use cases we could probably leave this unset (it defaults to row) with no impact. If a user added multiple toolbars then maybe it would be best to lay them out in a column rather than a row, but I don't see that as a supported use case so I'd vote to delete it for simplicity.
If I misunderstood the layout then please explain what this is achieving!
| it('should have a toolbar slot element in the shadow DOM', async () => { | ||
| await connect(); | ||
| const toolbarSlot = element.shadowRoot?.querySelector('slot[name="toolbar"]'); | ||
| expect(toolbarSlot).not.toBeNull(); | ||
| }); |
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.
@mollykreis for the input slot we're relying on the conversation's matrix tests to cover that behavior by including a state with the slot empty and a state with it populated. Same for the dialog footer being empty, which follows a similar pattern (see #2579 for discussion of the pattern). Since we have a similar matrix test for the toolbar slot, I'd advocate that it's ok to skip a unit test here too. Thoughts?
| "type": "minor", | ||
| "comment": "Add slot for toolbar in conversation component", | ||
| "packageName": "@ni/spright-components", | ||
| "email": "codruta.serban@ni.com", |
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.
Reminder that you can configure GitHub to hide your NI email if you want. See the instructions in CONTRIBUTING for more info. Once you do this you'll either have to delete and regenerate the change file or manually copy the secret GitHub email into this changefile.
Pull Request
π€¨ Rationale
Task 3739126: Add slot for toolbar in spright conversation
Add toolbar slot in conversation.
π©βπ» Implementation
Similar to input slot.
π§ͺ Testing
Manual testing and added test.
β Checklist