Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix/model selector improvements #5382

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix/model selector improvements #5382

wants to merge 3 commits into from

Conversation

actopas
Copy link

@actopas actopas commented Oct 14, 2024

Fix ModelSelector hover issues and HoverCard flicker

Hi, This PR addresses the issues reported in #5380, improving the user experience of the ModelSelector component.

Changes

  1. Fixed mouse hover event in ModelItem:

    • Added a stable div wrapper with ref to ModelItem
    • Implemented useEffect to manage mouseenter event listener
    • Ensured proper event listener cleanup on component unmount

  2. Eliminated HoverCard flicker in ModelSelector:

    • Added noAnimation prop to HoverCardContent
    • Set noAnimation to true for ModelSelector's HoverCard

Impact

These changes resolve two main issues:

  1. The hover state now correctly updates when moving between model items, even after filtering results.
  2. The HoverCard no longer flickers or shows unnecessary animation when interacting with the ModelSelector.

Testing

  • Tested hover functionality with various filter scenarios
  • Verified HoverCard stability across different interaction patterns
  • Ensured no regression in existing ModelSelector functionality

Screenshots

Check out

Additional Notes

This PR maintains the existing code structure while addressing the reported issues. The changes are minimal and focused on resolving specific problems without introducing new dependencies or major refactoring.

Closes #5380

@actopas
fix(components): resolve mouse hover event in ModelItem
9a5d320 
- Add stable div wrapper with ref to ModelItem
- Implement useEffect to manage mouseenter event listener
- Ensure proper event listener cleanup on component unmount
- Improve reliability of model peeking functionality
@actopas
fix(components): eliminate HoverCard flicker in ModelSelector
3740904 
- Add noAnimation prop to HoverCardContent
- Set noAnimation to true for ModelSelector's HoverCard
- Remove distracting animation effect for better user experience
- Maintain HoverCard functionality while improving visual stability
@vercel Vercel
Copy link

vercel bot commented Oct 14, 2024

@actopas is attempting to deploy a commit to the shadcn-pro Team on Vercel.

A member of the Team first needs to authorize it.

@Lexachoc
Copy link

@actopas Thank you for the PR. I jsut tested it. it works!

I noticed that the built-in keyboard navigation on the items does not trigger HoverCard.
Could you please support this? many thanks!

@Lexachoc
Copy link

@actopas I modified your good solution and now it supports the keyboard navigation!

No UseEffect. Just modify a bit of the original code

useMutationObserver(ref, (mutations) => {
    mutations.forEach((mutation) => {
        if (
            mutation.type === "attributes" &&
            mutation.attributeName === "aria-selected" &&
            ref.current.children[0]?.getAttribute("aria-selected") === "true"
        ) {
            onPeek(model)
        }
    })
})

change

ref.current?.getAttribute("aria-selected") === "true"

to

ref.current.children[0]?.getAttribute("aria-selected") === "true"

@actopas
fix(components): revert to use useMutationObserver and modify it
0bb0046 
- Reintroduce useMutationObserver to capture all selection changes, including keyboard navigation
- Improve handling of aria-selected attribute changes
- Ensure consistent behavior across mouse and keyboard interactions
@actopas
Copy link
Author

actopas commented Oct 15, 2024

@Lexachoc Thank you for your review. To reduce code changes and fix the missing feature of keyboard navigation, I switched useEffect back to useMutationObserver. Now the feature is working well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[bug]: Playground Model Combobox HoverCard bugs
2 participants
@actopas @Lexachoc