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

Switch out ReachUI's MenuButton for RadixUI's DropDownMenu #120

Merged
merged 2 commits into from Dec 9, 2021

Conversation

vitokhangnguyen
Copy link
Collaborator

@vitokhangnguyen vitokhangnguyen commented Dec 9, 2021

Changelog:

  • Re-implement Menu with RadixUI's DropDownMenu
  • Make Text component implement React.forwardRef
  • Revert UserMenu back to asynchronously rendering of MenuItems since Radix UI doesn't have the same bug as Reach UI
  • Temporarily disable reactStrictMode in next.config.js. See note below. Ignore the error on development for now (or you can right click on the error and choose to hide all errors from react-dom)

Note:

Currently, in development, react-dom would complain about Radix UI's mismatch id between client and server although IdProvider has wrapped around the entire app as this insturction from Radix suggests. This does not happens in production. The bug has been documented here by someone else.

Fortunately, a PR to resolve this has been merged 2 days ago. We could expect this issue to be fixed soon. A temporary solution to dismiss react-dom's complaining is to disable reactStrictMode. I'll create an urgent issue after this PR is merged to keep track of the fix (and docs) on Radix's side and update on our side asap. The update would also include re-enabling reactStrictMode.

Reflection:

  • Life just can't be easy
  • Choose lib better next time?

@vercel
Copy link

vercel bot commented Dec 9, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/andrewnt219/rent-near-me/JB4EJKwzkywtbGTitqpPuT8s8qfS
✅ Preview: https://rent-near-me-git-vitokhangnguyen-issue78menuv2-andrewnt219.vercel.app

Copy link
Owner

@Andrewnt219 Andrewnt219 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see we don't have to get around Reach implementation 👍.

src/ui/Menu/styles.ts Outdated Show resolved Hide resolved
next.config.js Outdated Show resolved Hide resolved
@Andrewnt219
Copy link
Owner

Andrewnt219 commented Dec 9, 2021

The grouping update is 💯.

image

@vitokhangnguyen
Copy link
Collaborator Author

The grouping update is 💯.

The only significant change that is visible to the user: I changed shadow to shadow-z8.

Cost: testng out 2 libraries, filing 1 open source issue, tracking 1 on-going open source PR, multiple code refactorings and sleeping schedule.

Worth it :)

@vitokhangnguyen vitokhangnguyen merged commit 1a3b7aa into main Dec 9, 2021
@vitokhangnguyen vitokhangnguyen deleted the vitokhangnguyen/issue78/MenuV2 branch December 9, 2021 19:20
@vitokhangnguyen vitokhangnguyen restored the vitokhangnguyen/issue78/MenuV2 branch December 9, 2021 19:21
@vitokhangnguyen vitokhangnguyen deleted the vitokhangnguyen/issue78/MenuV2 branch December 9, 2021 19:21
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.

None yet

2 participants