-
Notifications
You must be signed in to change notification settings - Fork 29
chore(ui): migrate demos from radio to select #540
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
chore(ui): migrate demos from radio to select #540
Conversation
Preview: https://chatbot-pr-chatbot-540.surge.sh A11y report: https://chatbot-pr-chatbot-540-a11y.surge.sh |
@rebeccaalpert I have not converted https://www.patternfly.org/patternfly-ai/chatbot/ui#welcome-message since it has only two options, using select will add more code then reduce. What are your thoughts? |
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.
Overall this looks great to me. There's a couple areas where we could stand to add <Stack hasGutter>
for better spacing. We'll also need to update screenshots, but I can do that after this merges. I pinged Erin for her comments.
packages/module/patternfly-docs/content/extensions/chatbot/examples/UI/ChatbotModal.tsx
Outdated
Show resolved
Hide resolved
packages/module/patternfly-docs/content/extensions/chatbot/examples/UI/ChatbotModal.tsx
Outdated
Show resolved
Hide resolved
packages/module/patternfly-docs/content/extensions/chatbot/examples/UI/TermsOfUse.tsx
Outdated
Show resolved
Hide resolved
packages/module/patternfly-docs/content/extensions/chatbot/examples/UI/TermsOfUseCompact.tsx
Outdated
Show resolved
Hide resolved
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.
Just one comment!
And then this would probably be a separate pr, but it would be nice to stack the checkboxes in https://chatbot-pr-chatbot-540.surge.sh/patternfly-ai/chatbot/ui#header-options, rather than have them in a horizontal list. Just didn't know where else to call this out
packages/module/patternfly-docs/content/extensions/chatbot/examples/UI/ChatbotContainer.tsx
Outdated
Show resolved
Hide resolved
packages/module/patternfly-docs/content/extensions/chatbot/examples/UI/ChatbotHeaderTitle.tsx
Outdated
Show resolved
Hide resolved
packages/module/patternfly-docs/content/extensions/chatbot/examples/UI/ChatbotModal.tsx
Outdated
Show resolved
Hide resolved
packages/module/patternfly-docs/content/extensions/chatbot/examples/UI/TermsOfUse.tsx
Outdated
Show resolved
Hide resolved
packages/module/patternfly-docs/content/extensions/chatbot/examples/UI/TermsOfUseCompact.tsx
Outdated
Show resolved
Hide resolved
14fbabc
to
70aea9e
Compare
Sorry for the delayed response, I have made the necessary changes. |
We currently have this implementation https://chatbot-pr-chatbot-540.surge.sh/patternfly-ai/chatbot/ui/#drawer-with-search-and-new-chat-button, would be a easy task |
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'll defer to Rebecca if we want to include the checkbox stacking in this or another PR. But this looks good!
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's do the checkboxes separately - I'll update screenshots after this merges.
🎉 This PR is included in version 6.3.0-prerelease.14 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes: #527