-
Notifications
You must be signed in to change notification settings - Fork 26
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
feat(UserFeedback): Add user feedback cards #409
Conversation
Preview: https://chatbot-pr-chatbot-409.surge.sh A11y report: https://chatbot-pr-chatbot-409-a11y.surge.sh |
9cd0f62
to
ca8fc57
Compare
ca8fc57
to
22cfcd3
Compare
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.
Sorry in advance for the wall of musing 😅 but, curious for your thoughts. And like I said, these can also just be thoughts for future follow ups. Doesn't have to hold back this pr!
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
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 <[email protected]>
…mples/Messages/Messages.md Co-authored-by: Erin Donehoo <[email protected]>
…mples/Messages/Messages.md Co-authored-by: Erin Donehoo <[email protected]>
…mples/Messages/Messages.md Co-authored-by: Erin Donehoo <[email protected]>
9674dbe
to
b682d17
Compare
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 is more from testing the docs out:
- The feedback buttons should have some sort of aria to convey whether they're selected/chosen/etc or not.
aria-expanded
would probably be fine considering they control the rendering of other content. - Since the tooltips of the feedback buttons has the same content as the button's aria-label, we should prevent the tooltip from describing the button, otherwise there could be duplicate announcement. The
aria="none"
prop on Tooltip should prevent it. - This could be a followup, but maybe providing a more unique aria-label on the close buttons of each feedback card? Nothing too verbose, but maybe "Close feedback for message received at [time]"?
- For the list of labels, the aria-label should be more descriptive of what the list is/contains. If it's a sort of "quick feedback" thing, something like "Quick feedback for message received at [time]" or something along those lines
- The textbox aria-label could also include a similar update as the previous 2 bullets, e.g. make it a little more unique with something like "Provide additional feedback for message received at [time]".
- For the feedback card without a close button, we'd want to remove the "optional" badge since the only way to get rid of the feedback card is to submit feedback. Unless the intent is that there would be a "cancel" action alongside the Submit button or similar
- For the timeout variation, we probably shouldn't have a timeout on a feedback card that can still be filled out, especially since it's such a short timeout and there may be no visual indication. Even though the timeout is prevented when hover or focus is on the card, it could still be problematic since it's content that can be filled out and possibly lost. Having a timeout on the "Thank you" card should be fine, but we'd also want to include verbiage about not setting the timeout below a certain threshold -- this may require some further research, though we could also ping design about whether using the same timeout lengths as tooltip/popover would be okay.
- Minor thing, but looks like there's a comma being rendered in the timeout example between the chatbot messages:
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 agree with Eric's comment about not including a timeout for the response cards so progress is not lost! Design looks good
1146608
to
a9624b2
Compare
packages/module/patternfly-docs/content/extensions/chatbot/examples/Messages/Messages.md
Outdated
Show resolved
Hide resolved
a9624b2
to
27c3e68
Compare
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.
Let me know if you have thoughts about any of these suggestions!
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
packages/module/patternfly-docs/content/extensions/chatbot/examples/demos/AttachmentDemos.md
Outdated
Show resolved
Hide resolved
packages/module/patternfly-docs/content/extensions/chatbot/examples/demos/Feedback.tsx
Outdated
Show resolved
Hide resolved
.../module/patternfly-docs/content/extensions/chatbot/examples/Messages/MessageWithFeedback.tsx
Outdated
Show resolved
Hide resolved
.../patternfly-docs/content/extensions/chatbot/examples/Messages/MessageWithFeedbackTimeout.tsx
Outdated
Show resolved
Hide resolved
packages/module/src/Message/UserFeedback/UserFeedbackComplete.test.tsx
Outdated
Show resolved
Hide resolved
packages/module/src/Message/UserFeedback/UserFeedbackComplete.test.tsx
Outdated
Show resolved
Hide resolved
.../module/patternfly-docs/content/extensions/chatbot/examples/Messages/MessageWithFeedback.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Erin Donehoo <[email protected]>
5bf4fee
to
ca123fb
Compare
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.
Looks great! Thanks for those changes
.pf-chatbot__feedback-card-title { | ||
font-family: var(--pf-t--global--font--family--heading); | ||
font-size: var(--pf-t--global--font--size--md); | ||
font-weight: 500; |
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.
Can you use a token like --pf-t--global--font--weight--body--bold
instead of hard coding 500?
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 is all set!
}, []); | ||
|
||
return ( | ||
/* card does not have ref forwarding; hence wrapper div */ |
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 open a PF issue to add ref forwarding to Card? Seems like an oversight.
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.
Adding issue here: patternfly/patternfly-react#11461
47209d5
to
f1de7e8
Compare
🎉 This PR is included in version 2.2.0-prerelease.13 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Ran had asked for a few things here:
I tried to support these and make this as configurable as possible. That said, I'm wondering if this may be too configurable and therefore annoying. Do want to add a plug-and-play easy-mode that just advances from the form to the thank you by itself, and also allow this for power users?
I tried to build in accessibility features, but I'm unsure if this goes far enough.
I defaulted the textarea to be opt-in. Do we want this opt-out? Let me know. I made close buttons, etc. fully configurable, and copied a lot of behavior from alerts. (Thanks to whoever wrote that awesome code and great tests.)
Docs: