-
Notifications
You must be signed in to change notification settings - Fork 9
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
Debt - 3952 - Replace primitives #4136
Conversation
Hmm, very odd. Something else that I do not remember touching in this PR. I just merged in main to see if I'm just a few commits behind or something 🤔 |
Looks like it was some bad merge conflict resolutions on my part. Should be good now in fix equity option titles
I'm not sure what one is correct but main is bold so I updated it to be bold in fix regular font weight delete action
Good eye! Added chevron in fix dropdown menu styles |
Nice! Overall it looks like things function just the same as before! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a big one, but as always great work Eric 🎉 Thanks for taking this one on, and again apologies for the long wait on the review.
Manual tests:
- Editing all touched sections of the Profile. Ensure all forms work behave and look correct.
- Login/Logout.
- Editing a pool.
- Table column and filter dialogs, and search dropdowns.
- User tabs
- Dialogs on IAP homepage.
Bugs:
- On the IAP homepage, the "See Eligibility Criteria" and "Learn more" dialog close buttons are not working.
- On the IAP homepage, when closing a dialog the tab index jumps back to the first element on the page, in this case the Home link in the nav menu. This can get annoying for users with screen readers. It seems to be working correctly on the Admin and TalentSearch side though.
- User cannot scroll when the dialog is open. This could prevent the user from seeing the entire Dialog, especially on smaller screens. For example, on the EE dialog I cannot click the Save button. Also, the "Learn more" dialog on IAP homepage is very long so most of the content is unviewable. Would adding a
<ScrollArea />
within the Dialog fix this? - The search form dropdown is missing on Pool Candidates table. ISSUE Feature - Pool Candidate table search filter #4621
- Clicking add user to pool breaks the page. It looks like it has something to do with the Input components (out of scope(?)). To recreate: go to Users table->View User->Click "add user to pool"
Cool stuff:
- Thanks for adding the docs for each of the new Primitives. It will come in handy down the line when ppl are using them for the first time.
- I like the subtle transition of color and the rotating chevron on the Accordion when opening and closing.
Good catch, fixed with fix iap dialog close + focus
Oops, this was a problem with the composition of the dialog. Thanks for catching that! Fixed in fix long content dialogs not scrolling
Looks like this was missed in the unsaved changes PR 😢 Fixed in fix add user to pool dialog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oohhh, good eye 🦅 Fixed with fix dialog close button position |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright looks good to me! ✅
Resolves #3952
📋 Summary
This replaces our underlying custom/reach
common
primitives withradix-ui
❓Rationale
Currently, we are stuck on
react@17
and the only real blocker isreach-ui
. After some research and looking at a number of primitive libraries, it seems as though radix is the best option because:✅ TO DO
🧪 Testing
npm run production --workspaces
npm run storybook
Places to Check
⚛️ Components
Accordion
AlertDialog
Collapsible
Dialog
DropdownMenu
ScrollArea
Separator
Switch
Tabs
ToggleGroup
Accordion
Alert Dialog
Dialog
Collapsible
Dropdown Menu
Scroll Area
Separator
Switch
Tabs
Toggle Group