-
Notifications
You must be signed in to change notification settings - Fork 12
Add chat cancel button #2856
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 chat cancel button #2856
Conversation
OliverCosma
left a comment
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 looks good to me, I'm waiting just for the textAreaKeydownHandler to be implemented :)
packages/storybook/src/spright/chat/input/chat-input.stories.ts
Outdated
Show resolved
Hide resolved
packages/angular-workspace/spright-angular/chat/input/spright-chat-input.directive.ts
Outdated
Show resolved
Hide resolved
packages/angular-workspace/spright-angular/chat/input/spright-chat-input.directive.ts
Outdated
Show resolved
Hide resolved
packages/storybook/src/spright/chat/input/chat-input.stories.ts
Outdated
Show resolved
Hide resolved
packages/spright-components/src/chat/input/tests/chat-input.spec.ts
Outdated
Show resolved
Hide resolved
| const processingStates = [ | ||
| ['not', false], | ||
| ['', true] | ||
| ] as const; |
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 other matrix stories for boolean attributes, it looks like we've established a pattern of naming the false state '' and giving the true state a value that represents its meaning. Let's keep with that pattern here.
| ] as const; | |
| const processingStates = [ | |
| ['', false], | |
| ['processing', true] | |
| ] as const; |
| " | ||
| > | ||
| ${valueLabel} value, ${placeholderLabel} placeholder | ||
| ${valueLabel} value, ${placeholderLabel} placeholder, ${processingLabel} processing |
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.
If you apply my suggestion above then this becomes unnecessary
| ${valueLabel} value, ${placeholderLabel} placeholder, ${processingLabel} processing | |
| ${valueLabel} value, ${placeholderLabel} placeholder, ${processingLabel} |
| return; | ||
| } | ||
| const eventDetail: ChatInputStopEventDetail = {}; | ||
| this.$emit('stop', eventDetail); |
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.
When there's no extra information to provide in the detail object, our typical pattern is to leave it unset. If you do this then you can also:
- revert the change to
types.ts - remove the export from the Angular directive
- remove the documentation about the type from storybook
| this.$emit('stop', eventDetail); | |
| this.$emit('stop'); |
| const eventDetail: ChatInputStopEventDetail = {}; | ||
| this.$emit('stop', eventDetail); | ||
| this.textArea?.blur(); | ||
| this.processing = false; |
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.
We didn't discuss this in the spec, but we should not not automatically clear the processing state when the Stop button is clicked. We generally avoid programmatically setting state (like attributes) that is also within the client app's control since it can cause their representation of that state to get out of sync. It is also good to stay symmetric: the client app added the processing attribute so the client app should be responsible for removing it.
| this.processing = false; |
| */ | ||
| public textAreaKeydownHandler(e: KeyboardEvent): boolean { | ||
| if (e.key === keyEnter && !e.shiftKey) { | ||
| if (e.key === keyEnter && !e.shiftKey && !this.processing) { |
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.
Thanks for catching that we don't want the Enter key to cause the send or stop button to be clicked. Though it's worth debating what we do want it do do:
- the behavior with the current implementation, which is to add a new line (behave like Shift-Enter)
- ignore it if you just press Enter, but add a new line if you press Shift-Enter
I think I have a personal preference for 2 because it feels more consistent. But I'm open to other preferences or to checking what desktop Nigel does and copying that.
Once you decide I think we should have a test for this case.
| ${x => x.sendButtonLabel} | ||
| <${iconPaperPlaneTag} slot="start"><${iconPaperPlaneTag}/> | ||
| </${buttonTag}> | ||
| ${when(x => !x.processing, html<ChatInput>` |
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.
Instead of swapping out the entire button when the processing attribute changes, I think it'd be cleaner to use the same button for both states but swap out the contents of the attributes that change. That would be less disruptive to the DOM contents and would allow the browser to preserve things like button focus.
Part of that would be using a more generic class for the button (e.g. class="action-button"). That would have some impacts on styles and the page object, but hopefully simplify them too.
| public sendButtonLabel?: string; | ||
|
|
||
| @attr({ attribute: 'stop-button-label' }) | ||
| public stopButtonLabel?: string; |
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.
Heads up that we have some tech debt where we want to move away from specifying user-visible strings as attributes and instead use label providers which have the advantage of allowing Nimble to provide a default value that apps can still localize.
I don't think we have to do this in this PR but it's something we should consider doing soon before we add too many more user-visible strings.
FYI @OliverCosma
Pull Request
π€¨ Rationale
https://dev.azure.com/ni/DevCentral/_workitems/edit/3740927
Add cancel button to chat input.
π©βπ» Implementation
Add
processing,stop-button-labelattributes andstopevent.π§ͺ Testing
Manual testing, updated tests and added matrix tests.
β Checklist