feat(chatbot conversation history nav): update history drawer icon/titling#588
feat(chatbot conversation history nav): update history drawer icon/titling#588rebeccaalpert merged 17 commits intopatternfly:mainpatternfly/chatbot:mainfrom Mash707:update-history-drawer-iconMash707/chatbot:update-history-drawer-iconCopy head branch name to clipboard
Conversation
|
Preview: https://chatbot-pr-chatbot-588.surge.sh A11y report: https://chatbot-pr-chatbot-588-a11y.surge.sh |
|
Added the clock icon and title prop. Spacing needs to be improved |
|
Fixed the spacing
|
rebeccaalpert
left a comment
There was a problem hiding this comment.
Thank you for the contribution! I think this is a good first start. I left some comments after poking around. If you feel comfortable and want to get more comfortable with tests, I think the new props would also benefit from some simple tests to check that the heading is there, customizable, etc. If you have questions about any of these things, just reach out to me here or on Slack.
packages/module/src/ChatbotConversationHistoryNav/ChatbotConversationHistoryNav.tsx
Outdated
Show resolved
Hide resolved
packages/module/src/ChatbotConversationHistoryNav/ChatbotConversationHistoryNav.tsx
Outdated
Show resolved
Hide resolved
packages/module/src/ChatbotConversationHistoryNav/ChatbotConversationHistoryNav.tsx
Outdated
Show resolved
Hide resolved
packages/module/src/ChatbotConversationHistoryNav/ChatbotConversationHistoryNav.tsx
Outdated
Show resolved
Hide resolved
| <div className="pf-chatbot__input"> | ||
| <Title headingLevel="h3"> | ||
| <Icon size="lg"> | ||
| <OutlinedClockIcon /> |
There was a problem hiding this comment.
I'm looking at our demo for "Drawer with simple menu" with these changes and I'm wondering if we might want to make this icon optional or customizable via props as well. We can do it as a follow-on after this merges.
|
@rebeccaalpert I am not much familiar with writing tests but looking at the existing ones I was able to write them. I am not sure of what props need to be passed. |
rebeccaalpert
left a comment
There was a problem hiding this comment.
Looks good to me! Thank you.
kaylachumley
left a comment
There was a problem hiding this comment.
Thank you so much for working on this! I left a few comments
packages/module/src/ChatbotConversationHistoryNav/ChatbotConversationHistoryNav.scss
Outdated
Show resolved
Hide resolved
packages/module/src/ChatbotConversationHistoryNav/ChatbotConversationHistoryNav.scss
Show resolved
Hide resolved
packages/module/src/ChatbotConversationHistoryNav/ChatbotConversationHistoryNav.scss
Show resolved
Hide resolved
|
I have updated the css for the title icon. |
rebeccaalpert
left a comment
There was a problem hiding this comment.
Looks good to me! Thank you so much.
| </DrawerActions> | ||
| </DrawerHead> | ||
| <div className="pf-chatbot__title-container"> | ||
| <Title headingLevel="h3"> |
There was a problem hiding this comment.
Thinking we should expose a prop to change the heading level. As it is axe flags this h3 as an issue due to "Heading levels should only increase by one" (updating it to an h2 in the DOM resolves the error in axe). Depending how/where the chatbot is placed, an h3 may not always be the correct heading level.
There was a problem hiding this comment.
I'll do this in a follow-on!
packages/module/src/ChatbotConversationHistoryNav/ChatbotConversationHistoryNav.tsx
Outdated
Show resolved
Hide resolved
3292d01 to
9e285d3
Compare
…on (patternfly#588) ChatBot user testing in Q1 2025 revealed that many users had trouble identifying the purpose of the chat history drawer. Design suggested adding a title to make this clearer to users. Title can be customized as needed. Co-authored-by: Rebecca Alpert <ralpert@redhat.com>
Closes: #510