tailwindlabs / headlessui

Completely unstyled, fully accessible UI components, designed to integrate beautifully with Tailwind CSS.
https://headlessui.com
MIT License
25.81k stars 1.07k forks source link

Merge incoming `style` prop on `ComboboxOptions`, `ListboxOptions`, `MenuItems`, and `PopoverPanel` components #3250

Closed RobinMalfait closed 4 months ago

RobinMalfait commented 4 months ago

This PR makes sure that incoming style props are merged in with the style prop that we provide.

We were overriding the style prop entirely on the <ComboboxOptions>, <ListboxOptions>, <MenuItems>, and <PopoverPanel> for anchoring purposes, as well as provided some CSS variables.

This now ensures that the incoming style prop gets merged in.

Fixes: #3248

vercel[bot] commented 4 months ago

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

Name Status Preview Comments Updated (UTC)
headlessui-react ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 28, 2024 1:35pm
headlessui-vue ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 28, 2024 1:35pm
RobinMalfait commented 4 months ago

Left a note on something but I've got another question… do we want our props to override theirs or the other way around?

That's a good question and it's a tough one. if you accidentally include style: { position: someCondition ? 'fixed' : undefined } then you might want fixed instead of the default absolute (coming from the anchor prop behavior), but the undefined will also win because it would override the internal absolute which would break the component.

Fwiw, currently we always override incoming props if we set certain props ourselves. Event handlers are always merged though. So based on this, I took the same approach.

There are some exceptions where we prefer incoming props (e.g.: an id) and there are places where we probably should prefer incoming props (e.g.: custom aria-label and aria-labelledby for example)

thecrypticace commented 4 months ago

yeah okay that seems reasonable. Given that undefined would also win (which I think would be undesirable) I think having ours override stuff is better. 👍