Conversation
|
Preview: https://chatbot-pr-chatbot-602.surge.sh A11y report: https://chatbot-pr-chatbot-602-a11y.surge.sh |
rebeccaalpert
left a comment
rebeccaalpert
left a comment
There was a problem hiding this comment.
Thank you for the contribution! I left a few comments below about the code. Overall it looks good to me.
If you could squash your commits and update your commit to be a format like feat(demo): Add demo for pinned chat history, that would be a big help. We trigger releases based on the magic word in front of the parentheses (fix and feat trigger a release). You'll also want to mention that you used Cursor in this - add something like the following to the commit: Assisted-by: Cursor. If you need help figuring out how to squash, just ping me and I can walk you through it.
|
@thatblindgeye - When you have time - do you think we need to do anything around focus here? Focus goes leaves when an item gets pinned or unpinned; it doesn't remain on the menu trigger. Do we want it to stay on the appropriate menu trigger? |
kaylachumley
left a comment
kaylachumley
left a comment
There was a problem hiding this comment.
Design wise, this functions as expected! looks good! thank you
rebeccaalpert
left a comment
rebeccaalpert
left a comment
There was a problem hiding this comment.
Thanks for making those changes - I think we just want to get Eric's feelings on focus (I would make that a follow-on issue, ideally). We just want to add Cursor into the final commit message, and I'll show you how to squash commits later if you have time.
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
@rebeccaalpert @melissaing per the ping above, yeah we should try to keep focus on the menu toggle that was accessed to pin the conversation in this demo. I might expect the flow to be:
- Click Conversation A meatball toggle
- Click the "Pin" option
- Conversation A is moved to the "pinned" section
- Focus is placed on Conversation A's meatball toggle again
melissaing
force-pushed
the
pin-unpin
branch
2 times, most recently
from
e52d413 to
91e7dcb
Compare
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Looks good! Opened #617 as a followup per discussion above
6 participants
Closes #511. Referenced other demos and pin implementation was assisted by Cursor. Updated UI.md with a new section and short description.