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

feat: Implement DropdownMenu Primitive in TokenSelectDropdown #1903

Merged
merged 2 commits into from
Feb 4, 2025
Merged

feat: Implement DropdownMenu Primitive in TokenSelectDropdown #1903

merged 2 commits into from
Feb 4, 2025

Conversation

cpcramer
Copy link
Contributor

@cpcramer cpcramer commented Jan 30, 2025 •
edited
Loading

What changed? Why?
This PR replaces the custom dropdown implementation in TokenSelectDropdown with the DropdownMenu primitive component, improving accessibility and maintainability.

Key Changes:

  • Replace manual click/blur handling with DropdownMenu's built-in event management
  • Remove custom positioning logic in favor of DropdownMenu's positioning system
  • Simplify state management to single isOpen toggle
  • Add proper ARIA attributes through DropdownMenu primitive
  • Maintain existing visual styling and TokenRow/TokenSelectButton components

Benefits:

  • Improved accessibility with proper ARIA attributes and keyboard navigation
  • Consistent positioning behavior across different screen sizes
  • Reduced code complexity by leveraging built-in primitive features
  • Better focus management and keyboard interaction
  • Standardized click-outside handling and escape key support
  • Portal-based rendering to prevent z-index issues
Screenshot 2025-02-03 at 2 20 08 PM

Notes to reviewers

How has it been tested?

@vercel Vercel
Copy link

vercel bot commented Jan 30, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
onchainkit-coverage ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 3, 2025 10:54pm
onchainkit-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 3, 2025 10:54pm
onchainkit-routes ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 3, 2025 10:54pm

@cpcramer cpcramer changed the title feat: Implement DropdownMenu Primitive in feat: Implement DropdownMenu Primitive in TokenSelectDropdown Jan 30, 2025
@cpcramer cpcramer force-pushed the paul/implement-dropdown-menu-into-token-selector branch from fcd3a8e to afbf087 Compare January 30, 2025 21:29
@cpcramer cpcramer mentioned this pull request Feb 3, 2025
Merged
@cpcramer cpcramer force-pushed the paul/implement-dropdown-menu-into-token-selector branch from 11876a1 to d94918f Compare February 3, 2025 21:49
@cpcramer cpcramer force-pushed the paul/implement-dropdown-menu-into-token-selector branch from d94918f to 34669a2 Compare February 3, 2025 21:50
@cpcramer cpcramer force-pushed the paul/implement-dropdown-menu-into-token-selector branch from 34669a2 to e0cd5f1 Compare February 3, 2025 21:53
@cpcramer cpcramer force-pushed the paul/implement-dropdown-menu-into-token-selector branch from 3dd39ee to e18f007 Compare February 3, 2025 22:18
@cpcramer cpcramer requested a review from brendan-defi February 3, 2025 22:21
@cpcramer cpcramer force-pushed the paul/implement-dropdown-menu-into-token-selector branch from 8b2caa7 to f55971d Compare February 3, 2025 22:42
@cpcramer cpcramer force-pushed the paul/implement-dropdown-menu-into-token-selector branch from f55971d to ba0620b Compare February 3, 2025 22:50
@cpcramer cpcramer requested a review from brendan-defi February 3, 2025 22:50
@cpcramer cpcramer merged commit 773ccbf into main Feb 4, 2025
16 checks passed
@cpcramer cpcramer deleted the paul/implement-dropdown-menu-into-token-selector branch February 4, 2025 17:50
@github-actions github-actions bot mentioned this pull request Feb 4, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brendan-defi brendan-defi brendan-defi approved these changes

@alessey alessey alessey approved these changes

Assignees
No one assigned
Labels
pkg: token
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cpcramer @alessey @brendan-defi