-
Notifications
You must be signed in to change notification settings - Fork 29
feat(SourcesCard): Add expanded view #501
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(SourcesCard): Add expanded view #501
Conversation
Preview: https://chatbot-pr-chatbot-501.surge.sh A11y report: https://chatbot-pr-chatbot-501-a11y.surge.sh |
6ff86f8
to
95821ba
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.
left a comment! looking really 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.
we currently say in the md "The title is limited to 1 line and the body is limited to 2 lines.", which this is an exception to.
Should we just rephrase that to be "For the best clarity and readability, we strongly recommend limiting the title to 1 line and the body to 2 lines.
Q: Is there a max length we want to advise
...s/module/patternfly-docs/content/extensions/chatbot/examples/Messages/MessageWithSources.tsx
Outdated
Show resolved
Hide resolved
0e026a4
to
9792e32
Compare
8342979
to
e791aa7
Compare
e791aa7
to
5432c4c
Compare
{ | ||
title: 'Getting started with Red Hat OpenShift', | ||
link: '#', | ||
body: 'Red Hat OpenShift on IBM Cloud is a managed offering to create your own cluster of compute hosts where you can deploy and manage containerized apps on IBM Cloud ...', |
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.
body: 'Red Hat OpenShift on IBM Cloud is a managed offering to create your own cluster of compute hosts where you can deploy and manage containerized apps on IBM Cloud ...', | |
body: 'Red Hat OpenShift on IBM Cloud is a managed offering to create your own cluster of compute hosts where you can deploy and manage containerized apps on IBM Cloud ...', |
I can't remember if we explicitly decided last week -- do we want to take the ellipses off of these examples?
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 removed the ellipses on the "show more" example, but left the others alone. The ellipsis in the text don't actually get shown in the other examples. It's a matter of taste whether we remove them or not. This is just a test fwiw.
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.
Could you update the md docs to change:
"The title is limited to 1 line and the body is limited to 2 lines."
to
"For the best clarity and readability, we strongly recommend limiting the title to 1 line and the body to 2 lines. If the body description is more than 2 lines, use the "long sources" or "very long sources" variant."
Docs change all set! |
Allow for cards with more source info.
fdb7e75
to
e8bb8af
Compare
i didn't get requested for a second review but it looks good! @rebeccaalpert |
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.
lgtm but have a general question about the message sources examples.
It doesn't seem like there's a large difference between the "Example with sources" and "Example with very long sources". And so with the new "Example with long sources" the very-long-sources example now looks a little off, since I would expect a similar show more/less or other feature. Should we maybe combine or remove one of these?
Edit: or if the very-long-sources is actually showcasing the title truncation + tooltip, that makes sense, but it's a little unclear based on the wording
Refines example content for messages with sources.
@kmcfaul - Erin just pushed some changes - hopefully a little bit more clear now. |
🎉 This PR is included in version 2.2.0-prerelease.45 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Allow for cards with more source info.
Docs: https://chatbot-pr-chatbot-501.surge.sh/patternfly-ai/chatbot/messages#messages-with-sources