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

Allow custom id attribute to <Listbox.Button /> #2020

Closed
wants to merge 1 commit into from

Conversation

mgcrea
Copy link
Contributor

@mgcrea mgcrea commented Nov 16, 2022

Needed to use an external label for my <Select />-like component (In my case the label is part of a generic <FormItem /> that manages validation for form items). And unfortunately you currently can't override the <Listbox.Button /> id attribute. This PR fixes that.

@vercel
Copy link

vercel bot commented Nov 16, 2022

@mgcrea is attempting to deploy a commit to the Tailwind Labs Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Nov 16, 2022

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

Name Status Preview Updated
headlessui-react ✅ Ready (Inspect) Visit Preview Nov 16, 2022 at 5:07PM (UTC)

@RobinMalfait
Copy link
Member

Hey! Thank you for your PR!
Much appreciated! 🙏

I'm going to say no to this PR for now because this will leave it in an inconsistent state for all other components. In some components it isn't as easy to set the id right now (because it comes from state via context from the root component).

Would using our Listbox.Label component instead work for you?
https://headlessui.com/react/listbox#using-a-custom-label

Thanks!

@mgcrea
Copy link
Contributor Author

mgcrea commented Nov 25, 2022

Would using our Listbox.Label component instead work for you?

Unfortunately I can't use the Listbox.Label because my label is not inside the Listbox, it's a sibling managed by another component.

Setting an id does not make sense for most components but for this one it does since you need it to leverage the native focus on click. Would merge this as there is no breaking change at all and is a one-liner for a valid use-case.

RobinMalfait added a commit that referenced this pull request Dec 2, 2022
Continuation of #2020
Co-authored-by: Olivier Louvignes <[email protected]>
RobinMalfait added a commit that referenced this pull request Dec 2, 2022
RobinMalfait added a commit that referenced this pull request Dec 2, 2022
* accept `id` as a prop where it is currently hardcoded (React)

Continuation of #2020

Co-authored-by: Olivier Louvignes <[email protected]>

* accept `id` as a prop where it is currently hardcoded (Vue)

* update changelog

* apply React's hook rules

Co-authored-by: Olivier Louvignes <[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.

2 participants