Conversation
change/@ni-spright-angular-3dd80b72-fa3a-4f62-9749-f852ef6a2b91.json
Outdated
Show resolved
Hide resolved
...ce/spright-angular/chat/message/welcome/tests/spright-chat-message-welcome.directive.spec.ts
Show resolved
Hide resolved
packages/spright-components/src/chat/message/welcome/tests/chat-message-welcome.spec.ts
Outdated
Show resolved
Hide resolved
packages/spright-components/src/chat/message/welcome/template.ts
Outdated
Show resolved
Hide resolved
| This is the message text of this banner. It tells you something interesting. | ||
| </${bannerTag}> | ||
| `)} | ||
| ${when(x => x.welcome, html<ChatConversationArgs>` |
There was a problem hiding this comment.
Looks like in this way we have the welcome messages along with the other messages
My suggestion would be to have a separate story with the welcome message, given that in practice we will have login and then display the rest of the chat conversation
There was a problem hiding this comment.
@jattasNI - there is just one story with the entire "chat conversation". The system/inbound/other types of messages appear in the subsection chat messages. However, to me it feels weird to have the welcome messages in the chat conversation story - would it make sense to have a separate story just with the chat conversation - welcome message - disclaimer - and the login button ?
There was a problem hiding this comment.
I like having the welcome message in a separate story: it doesn't feel right to put it in a conversation next to the other messages. But I think it does make sense to include it in the conversation MDX so that it is documented next to other messages and chat components.
If we stick with that approach then I don't think we need the new welcomeLogin story in chat-conversation.stories.ts. The messageStories.chatMessageWelcome that's being embedded in the MDX and included in the Chromatic diff should be sufficient.
packages/storybook/src/spright/chat/message/chat-message-matrix.stories.ts
Outdated
Show resolved
Hide resolved
packages/storybook/src/spright/chat/message/chat-message-matrix.stories.ts
Outdated
Show resolved
Hide resolved
| loginButton: { | ||
| name: 'Show login button', | ||
| description: 'Slot a login anchor button below the title and subtitle.', | ||
| table: { category: apiCategory.slots } | ||
| }, | ||
| suggestions: { | ||
| name: 'Show suggestions', | ||
| description: 'Slot suggested outbound messages buttons below the title and subtitle.', | ||
| table: { category: apiCategory.slots } | ||
| } |
There was a problem hiding this comment.
We try to keep a 1-1 mapping between parameter names and actual parameters. In this case I'd like to show a single slot parameter with name: 'default' rather than multiple slots with names "Show login button" and "Show suggestions".
The way we've achieved that in other places is to change the type of the slot to be an enum that lets you pick between different types of higher-level content. In this case you could show all or some of:
- "None" - equivalent to both login button and suggestions being false
- "Login button"
- "Suggestions"
- "Login button and suggestions"
You could skip any combinations that don't make sense (e.g. maybe we shouldn't include the last one because you shouldn't be able to receive suggestions if you're not logged in).
You can see an example of this in the breadcrumb story's docs for its default slot. https://nimble.ni.dev/storybook/?path=/docs/components-breadcrumb--docs
| gap: ${mediumPadding}; | ||
| } | ||
|
|
||
| .brand-icon { |
There was a problem hiding this comment.
From screenshots of desktop and/or wireframes I thought I recalled this icon being big and multicolored rather than small and only black. I can confirm what's right at the UX meeting this Friday, but to start I'd suggest matching what desktop is doing:
- images (there's also a dark version)
- image sizing
From this PR
From Figma
| 'Chat below to get started' | ||
| )); | ||
| await connect(); | ||
| const subtitleDiv = element.shadowRoot?.querySelector('.subtitle'); |
There was a problem hiding this comment.
Could you put all the DOM manipulation code into a page object? Should include querySelector, textContent, trim, etc.
| expect(subtitleDiv).toBeNull(); | ||
| }); | ||
|
|
||
| it('should have a brand-icon slot', async () => { |
There was a problem hiding this comment.
I think we're missing a test about the default brand icon.
4 participants
Pull Request
https://dev.azure.com/ni/DevCentral/_workitems/edit/3786413
Continuation of #2891
🤨 Rationale
👩💻 Implementation
🧪 Testing
✅ Checklist