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.

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

refactor: migrate dropdown menu to zeego/dropdown-menu #2696

Merged
merged 3 commits into from
Feb 6, 2025
Merged

Conversation

lawvs
Copy link
Member

@lawvs lawvs commented Feb 5, 2025

Integrate the Zeego dropdown menu into the Select and SortActionButton components, while removing the old DropdownMenu component and its dependency on react-native-context-menu-view.

Screenshot 2025-02-06 at 00 44 14 Screenshot 2025-02-06 at 00 44 28

Doc: https://zeego.dev/components/dropdown-menu

Copy link

vercel bot commented Feb 5, 2025

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

Name Status Preview Comments Updated (UTC)
follow ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 5, 2025 4:53pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
follow-external-ssr ⬜️ Ignored (Inspect) Visit Preview Feb 5, 2025 4:53pm

@follow-reviewer-bot
Copy link

Suggested PR Title:

refactor(dropdown): update Select to use zeego component

Change Summary:
Refactored dropdown functionality and removed deprecated components. Updated Select component to use a new dropdown menu from 'zeego/dropdown-menu'. Improved subscription header actions for sorting functionality, enhancing the overall UI/UX of dropdown interactions.

Code Review:

Code Review: Change Requests

1. File: apps/mobile/src/components/ui/form/Select.tsx

  • Lines 41-74: The usage of <DropdownMenu.CheckboxItem> for a selection mechanism implies that multiple values could be selected simultaneously. However, this component is implementing a single-select dropdown (Select<T>). Using <CheckboxItem> for single-selection creates a semantic mismatch and could lead to unexpected behavior. Replace <CheckboxItem> with an appropriate RadioItem or equivalent API from zeego/dropdown-menu that supports single-selection to better convey the intent both visually and in code.

2. File: apps/mobile/src/modules/subscription/header-actions.tsx

  • Lines 23-94: Similar to the previous file, the <DropdownMenu.CheckboxItem> is being used for sub-actions in the dropdown menu. Since the SortActionButton indicates that only one option can be selected at a time (either Ascending or Descending), using <CheckboxItem> can create confusion. Replace <CheckboxItem> with an appropriate single-selection item, like RadioItem or Item, to reflect the correct choice semantics.
  • Lines 54-55: The use of key values for nested dropdown items includes dynamic generation patterns like key={'SubContent/' + action.title + '/' + subAction.title}. Dynamically concatenated keys can lead to problems if action.title or subAction.title contain characters like spaces or special symbols. Use a more rigorous key generation approach to ensure keys are unique and consistent across all use cases (e.g., by hashing or normalizing titles).

3. General Concerns:

  • Deprecation/Backward Compatibility: The migration from react-native-context-menu-view to zeego/dropdown-menu seems to be a significant architectural shift. Ensure there are no existing dependencies or UI components still relying on the react-native-context-menu-view package before its removal from package.json. If any dependencies exist, they need to be refactored or replaced before merging this change.

Summary of Requested Changes

  1. Replace <CheckboxItem> with a more appropriate single-select component (e.g., <RadioItem>) in both Select.tsx and header-actions.tsx.
  2. Use a safer, consistent key generation strategy for dropdown menu items in header-actions.tsx.
  3. Double-check for dependencies on the removed react-native-context-menu-view package and refactor or replace them if they exist.

Let me know if further clarification is needed!

@lawvs lawvs force-pushed the refactor/drop-down branch from 40cd915 to 47d59f8 Compare February 5, 2025 16:51
@lawvs lawvs merged commit 5d6386f into dev Feb 6, 2025
10 checks passed
@lawvs lawvs deleted the refactor/drop-down branch February 6, 2025 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant