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

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

Merged
merged 2 commits into from
May 28, 2024

Conversation

RobinMalfait
Copy link
Member

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

Copy link

vercel bot commented May 28, 2024

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

Copy link
Contributor

@thecrypticace thecrypticace left a comment

Choose a reason for hiding this comment

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

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

packages/@headlessui-react/src/components/menu/menu.tsx Outdated Show resolved Hide resolved
@RobinMalfait
Copy link
Member Author

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
Copy link
Contributor

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. 👍

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.
@RobinMalfait RobinMalfait merged commit 94bc4e1 into main May 28, 2024
8 checks passed
@RobinMalfait RobinMalfait deleted the feat/issue-3248 branch May 28, 2024 13:37
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.

style prop of ComboboxOptions is ignored
2 participants