-
Notifications
You must be signed in to change notification settings - Fork 9
Update Spright chat spec to add input component #2611
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?
Conversation
- the text area height is a single line initially but grows its height to fit the entered text up to its max-height | ||
- the text area has configurable placeholder text | ||
1. Includes a "Send" button for the user to submit the current input text | ||
- fires an event containing the current input content and then clears the content and sets keyboard focus back to the input |
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 makes me a bit nervous. The general vague feeling is that it's encoding too much behavior that's different from normal text areas.
Specific concerns is that it feels like it wouldn't align with validation workflows. i.e. on submit? character entry? etc maybe we want to show error-text on the input and disable submission.
Native forms have a reset concept which is kind of similar. Maybe we have a method like that, i.e. resetInput` or something?
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.
Addressed this with the new API proposed in the other thread.
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.
@rajsite, just pointing your attention to the fact that I have pushed back on this, and have reverted the spec to how Jesse originally had, with some additional comments in the 'Input API alternatives' section. So, in summary, the resetInput()
method has been removed, and pressing 'Send' (or <Enter>) will cause the text to clear.
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.
Out of office, so just some passing feedback. Could we make that behavior opt-in instead of the default? I.e. a reset-on-send
attribute or something.
The designs today don't include error-text and validation patterns which I'm concerned auto clearing will fight against, but if it's opt-in that seems less of a concern.
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.
The two types of errors I think are expected are things external to the chat input, like network issues, in which case the outbound message was still sent (and thus no need to preserve it in the input area), and internal to the chat input, such as with an invalid file attachment, in which case the design is to disable the send button.
So, there are no known use-cases that there is cause for concern for always clearing the text input on send. If we ultimately find that this is incorrect, it will not be hard to pivot and update the small set of clients that would be impacted.
|
||
This is included for informational purposes but doesn't yet have a proposed implementation in the Design section below. | ||
|
||
1. Includes a "Stop" button for the user to abort an action that was triggered in response to a sent message (e.g. a file upload or incoming response) |
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.
Maybe we'd have some state and transitions based on the comment above. i.e.
idle (can accept input) -(error-visible)-> error (submit disabled)
idle (can accept input) -(user submit)-> processing (submitted and loading) -(resetInput
)-> idle (can accept input)
idle (can accept input) -(user submit)-> processing (submitted and loading) -(error-visible)-> error -(some user acknowledgement of error button? + resetInput
)-> idle (can accept input)
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 took my own stab at designing what this would look like. I think I landed similar to your proposed states. Might be worth a call to discuss once you've taken a look.
These are the states I can see in the current design spec:
- "Idle". Accepting input but send disabled due to empty input.

- "Active". Accepting input with send enabled due to populated input or attachment. Note: if our API for attachments is client-populated slot content, then the input component probably can't automatically detect that state.


- "Error". Accepting input but send disabled due to an error

- "Processing". Accepting input but send changed to "Stop" due to processing a previous input.

Some considerations:
- I don't want clients to have to implement too much logic to determine which state to put the component in.
- For example, related to How can I handle the Enter key on NimbleTextArea and get the latest value?Β #2551 I'd rather not have clients care about Enter vs Shift Enter.
- I'd rather not have clients respond to every key press to enable/disable the Send button
- I don't want to have a different error API than other components (
error-visible
)
Proposed API:
state = "default" | "processing"
- controls whether to show Send or Stopsend
event fires on Enter/button press but doesn't clear input contents or set focus or change button stateresetInput()
- client calls to clear input contents and set focus- Send button automatically enabled/disabled if non-empty/empty input.
send-button-enabled
andsend-button-disabled
to explicitly enable/disable it regardless of input state- Stop button always enabled if visible.
Happy path flow:
state = default
and Send button is visible but disabled because text is empty- user types text. Send is automatically enabled
- user presses Enter.
send
event is fired - client handles event and calls
resetInput()
Happy path flow with processing state:
1-3 same as above
4. client handles event and sets state = "processing"
to show Stop button. Also calls resetInput()
5. a) finishes processing. Client sets state = default
to swap button state
5. b) user presses Stop before finishes processing. Client handles event and sets state = default
User adds attachment:
- same as above
- user adds attachment. Client processes picked file and slots a chip/pill to represent it.
- Client sets
send-button-enabled
to override disabled button state due to empty input field - a) User clears attachment. Client removes chip/pill and clears
send-button-enabled
- b) User presses send with attachment. In event handler, client removes chip/pill and clears
send-button-enabled
and callsresetInput()
Client detects error:
- Client sets
error-visible
anderror-text
andsend-button-disabled
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 updated the API in the spec based on the above proposal plus some tweaks that Milan and I made in conversation.
- it now includes the things that were previously out of scope (attachment slots, stop button, error state)
- We opted to configure whether the stop button or send button was visible based on a simple boolean attribute. We also considered a readonly state attribute and methods to transition between them but that seemed overkill for now
- We got rid of the
send-button-enabled
event and proposed that the button would automatically be enabled if the attachment slot contains content
FYI @atmgrifter00 in case you want to review the new API
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.
Should we provide some accessibility expectations related to the Send/Stop button(s)? I imagine we want to ensure that keyboard focus stays on the button after being pressed (i.e. if 'Send' button is pressed, the 'Stop' button should now have keyboard focus and vice versa). Also, are there any accessibility concerns related to the function of the button changing (e.g. does it need to be announced)?
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.
Good questions.
The typical flow would be that after the user sent their text a client would call resetInput()
and optionally set the state to processing
. With the current proposal, resetInput()
would set focus back to the text area regardless of whether the user sent the text using Enter in the text area, clicked the Send button, or tabbed to the Send button and pressed Enter. That seems sane to me since the user likely wants to type another message more than they want to stop the current one from being processed.
If a client doesn't call resetInput()
at the same time they set the state to processing
then the entered text would remain in the text area. That definitely seems weird to me so I don't think we'd encourage clients to do that. We're not currently offering a way to clear the text but maintain the focus on whatever had it before. If we need to do that we could potentially pass some config options to resetInput()
to configure whether it should set focus. I agree that in that case you'd probably want the Stop button to get focus if Send had it before (and vice versa). But in the interest of scope I'd advocate to skip doing that for now.
Similarly I'd vote to punt on the question of announcing button changes (this is in Spright so we can be looser with our accessibility behavior). I don't know if users would find those announcements useful or annoying if they happened on every sent message.
I added a couple sentences to the Accessibility section to capture those proposals. Let me know if you disagree.
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.
@rajsite @mollykreis heads up that I'm starting implementation on the first vertical slice of this. If you have feedback on the parts that configure the text area and send button, it'd be good to get agreement before I write tests.
The stop button, error state, and slots are out of scope for my first slice of implementation.
# Pull Request ## π€¨ Rationale This is the first vertical slice of the component in #2611. Part of #2610. This adds the `spright-chat-input` component and Angular wrapper. The component supports: - user input (including multi-line via Shift-Enter) - send button (and Enter key to send) - resetInput() method to clear input after send - placeholder text Coming in future PRs: - Blazor wrapper - styling for states like hover/focus - error state - stop button and "processing" state - slots for additional content like attachments ## π©βπ» Implementation Pretty standard component and Angular integration, as described in the HLD. I'll mention some interesting notes in comments. ## π§ͺ Testing 1. Unit tests for component and Angular directive 2. Matrix tests for component 3. Added to Angular example app and Storybook ## β Checklist - [x] I have updated the project documentation to reflect my changes or determined no changes are needed.
β¦button (or pressing Enter) clears text.
# Pull Request ## π€¨ Rationale The design for the chat input calls to _always_ clear the text in the input once a user sends the message. There does not seem to be a compelling enough reason to make the client call a method in response to the `send` event to make this happen ([additional context](#2611 (comment))). ## π©βπ» Implementation Make `resetInput` private and call it from the `sendButtonClickHandler`. ## π§ͺ Testing Unit test. ## β Checklist <!--- Review the list and put an x in the boxes that apply or ~~strike through~~ around items that don't (along with an explanation). --> - [ ] I have updated the project documentation to reflect my changes or determined no changes are needed.
Pull Request
π€¨ Rationale
This is the design doc for the first task in #2610
π©βπ» Implementation
Update existing spec to propose a new component,
spright-chat-input
.π§ͺ Testing
N/A
β Checklist