feat(ResponseActions): Add option for persistent selections #740
Conversation
|
Preview: https://chatbot-pr-chatbot-740.surge.sh A11y report: https://chatbot-pr-chatbot-740-a11y.surge.sh |
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Couple quick nits below. Overall I think this is getting closer to what I'd expect, I'm just not sure if it's more an example imeplementation or how the logic is setup. Kind of a brain dump here so if you'd want I'd be happy to hop on a call or anything.
Currently in this PR:
- Clicking an action in the new example persists it's selection
- Ciicking outside doesn't deselect that action
- Clicking another action deselects the previous action and selects the new action
- This applies to all actions in the example, as all the actions are really in a single group
What I think would be better to convey in an example is:
-
Only some actions can be persisted and grouped. So the 2 response actions are grouped together and will persist unless clicking the other response, and clicking any other action won't deselect these actions. Similarly, the "Listen" action could be in its own group as well. So basically ResponseActionGroup that is indepedent of any other actions or groups in ResponseActions, retaining their selected stated when interacting outside the group and only updating when interacting inside the group.
-
We could have a followup for this, but I think having a means other than the color/opacity of the icon changing on selection would be great. 2 icons (one default, one selected) might be good, but would require discussion with design.
-
This is sort of an existing issue but could be something to tweak in this new "persisted" imeplementation for now, but the aria-labels and how they're handled could be improved for actions as a whole (most likely just example updats). For non-persistent actions, I think it'd make sense to update actions to their default state upon moving focus from them, or tweaking the aria labels/tooltips. Clicking a response action for example changes the aria-label and tooltip to "Response recorded", which takes away the context of the type of response (good, bad, maybe a third or fourth option) if you navigate between actions without clicking anything else.
In the case of persisted actions it's a bit worse since the labels/tooltips won't reset unless you click another action. So clicking "Good response" changes the action to a "Response recorded" action essentially, which might not be the worst in our examples where there's only 2 actions, but still not great and if there's ever more it can get easily confusing.
A couple of options could be tweaking the labels/tooltips to keep the default action name and adding some verb at the end ("Good response recorded" maybe, the "Listen" action is probably fine as-is since its default and "selected" labels are similar), or we could just keep the default label/tooltip and add an aria-pressed attribute (so "Good response" is the accessible name and tooltip regardless of if it's selected or not, but the aria-pressed of true and false will provide the context of whether that action is selected or not).
-
Something I noticed that is pre-existing so wew can open a followup issue for if you can confirm it, but when clicking an action with the Space key I'm able to have the tooltip remain active and see the updated tooltip content, but when pressing Enter it removes the tooltip.
Like I said, whether some of these things are more with how we have the example implemented or whether it's stuff that should live in the actual component logic may depend.
packages/module/patternfly-docs/content/extensions/chatbot/examples/Messages/Messages.md
Outdated
Show resolved
Hide resolved
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Forgot that #581 is open for handling some of my comment above. To clean up my previous comment, what I think we should then do is (for now going with the assumption we only want to show off persisted actions and how to do just that; any other mechanisms for grouping etc will be covered in 581):
- For this new example, tweak the accessible names of the actions as applicable
- We should ensure that regardless of selected state, an actions purpose isn't lost. This would really just involve either having these persisted actions have the same accessible name and tooltip regardless of selected state and leverage
aria-pressed, or just have the actions have an accessible name and tooltip like "[action] [verb]", so "Good response recorded".The "Listen" action could probably remain the same. - This is more a nit, but maybe for this new example we remove the copy and download actions? Not sure if those really make sense to persist and idk if we want to recommend it.
- We should ensure that regardless of selected state, an actions purpose isn't lost. This would really just involve either having these persisted actions have the same accessible name and tooltip regardless of selected state and leverage
- Ideally I think we should open a followup to handle reverting action accessible names/tooltips to their default when focus moves away from them for non-persisted actions only
rebeccaalpert
force-pushed
the
response-actions-selection
branch
2 times, most recently
from
d90facc to
42099c9
Compare
Assisted-by: Cursor
rebeccaalpert
force-pushed
the
response-actions-selection
branch
from
42099c9 to
8efd34f
Compare
|
I think I got the feedback, but let me know if there's something I'm missing here. |
edonehoo
left a comment
edonehoo
left a comment
There was a problem hiding this comment.
some slight changes, but lmk if you want to tweak any of them further!
packages/module/patternfly-docs/content/extensions/chatbot/examples/Messages/Messages.md
Outdated
Show resolved
Hide resolved
packages/module/patternfly-docs/content/extensions/chatbot/examples/Messages/Messages.md
Outdated
Show resolved
Hide resolved
packages/module/patternfly-docs/content/extensions/chatbot/examples/Messages/Messages.md
Outdated
Show resolved
Hide resolved
…mples/Messages/Messages.md Co-authored-by: Erin Donehoo <105813956+edonehoo@users.noreply.github.com>
rebeccaalpert
force-pushed
the
response-actions-selection
branch
from
53fefe2 to
b43d0b7
Compare
| ref={negative.ref} | ||
| aria-expanded={negative['aria-expanded']} | ||
| aria-controls={negative['aria-controls']} | ||
| aria-pressed={persistActionSelection ? activeButton === 'negative' : undefined} |
There was a problem hiding this comment.
It may not actually be terrible with the default values of the selected labels, but in general we would want to have either a dynamic accessible name or use aria-pressed, not both. Having both an accessible name and aria-pressed update can make it confusing in terms of the current state/expected update.
I think the update to the default selected labels are good here. What do you think about doing this:
- in this PR, we just remove the aria-pressed
- We open a followup to allow a consumer to pass aria-pressed in for an action themselves, for a scenario where maybe they just want "Good response" to always be the label and tooltip but still have a means of conveying it's selected or not
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
LGTM, I opened #755 just for quick docs updates for aria-pressed. It's currently possible to pass it, just we should provide documentation.
|
🎉 This PR is included in version 6.5.0-prerelease.14 🎉 The release is available on: Your semantic-release bot 📦🚀 |
5 participants
This seemed like a perfect ticket for robots to take a crack at and I massaged the bits that didn't work 100%.
Demo: https://chatbot-pr-chatbot-740.surge.sh/patternfly-ai/chatbot/messages#message-actions-selection-options
Assisted-by: Cursor