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

UXIT-1428 - Headless UI v2 component refactor #701

Merged
merged 12 commits into from
Oct 16, 2024

Conversation

barbaraperic
Copy link
Collaborator

📝 Description

The V2 Headless UI components no longer use the <Parent.Child> component pattern, and rely on a different props API.

  • Type: Refactor

🧪 How to Test

Check the following and see if everything works as intended (there should be no changes):

  • Project Form
  • Navigation (desktop and mobile)

🔖 Resources

@barbaraperic barbaraperic self-assigned this Oct 9, 2024
Copy link

vercel bot commented Oct 9, 2024

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

Name Status Preview Comments Updated (UTC)
filecoin-foundation-site ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 16, 2024 1:32pm

Copy link

Copy link
Collaborator

@mirhamasala mirhamasala left a comment

Choose a reason for hiding this comment

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

Looking good! 🙌🏼 Nit: I would rename everything to include the full name by adding UI so HeadlessUIListboxButton, etc.

I think we can also run npm install @headlessui/react@latest at this point.

Tagging @CharlyMartin for a second look. 🙏🏼

Copy link
Collaborator

@CharlyMartin CharlyMartin left a comment

Choose a reason for hiding this comment

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

@barbaraperic I think we can remove most aria labels as they are managed by Headless UI now. Also, in some instances, we should import their type definition to be closer to reality.

Let me know what you think :)

Thanks!


As a more general comment, we might want to find a way to use FormListbox here. The implementation of both components is very similar, but that's probably for another PR!

src/app/_components/ListboxOptions.tsx Outdated Show resolved Hide resolved
src/app/_components/ListboxOption.tsx Outdated Show resolved Hide resolved
src/app/_components/ListboxButton.tsx Outdated Show resolved Hide resolved
src/app/_components/SlideOver.tsx Show resolved Hide resolved
@CharlyMartin CharlyMartin force-pushed the bp/update-headless-ui-components branch from c159454 to a5832bd Compare October 16, 2024 13:29
@CharlyMartin
Copy link
Collaborator

CharlyMartin commented Oct 16, 2024

@mirhamasala I got rid of the Transition component in Popover but couldn't manage to do the same for the Dialog. I didn't want to spend too much time on this, so I believe it's good to go for now!

Copy link
Collaborator

@mirhamasala mirhamasala left a comment

Choose a reason for hiding this comment

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

@barbaraperic @CharlyMartin - Love it! So much cleaner! ❤️ LGTM!

@barbaraperic barbaraperic merged commit c620db6 into main Oct 16, 2024
5 checks passed
@barbaraperic barbaraperic deleted the bp/update-headless-ui-components branch October 16, 2024 14:06
mirhamasala added a commit that referenced this pull request Oct 17, 2024
* refactor headless ui component v2

* replace headless with headlessUI and fix active to focus

* remove aria label and implement data selectors

* upgrade headless version and remove aria lables

* fixup! refactor headless ui component v2

* Remove Transition component from Popover

---------

Co-authored-by: Mirha Masala <[email protected]>
Co-authored-by: Charly MARTIN <[email protected]>
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.

3 participants