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

[Bug]: MenuLabel rendered as <label>, should be <div> #482

Closed
madebyfabian opened this issue Oct 30, 2023 · 6 comments · Fixed by #486
Closed

[Bug]: MenuLabel rendered as <label>, should be <div> #482

madebyfabian opened this issue Oct 30, 2023 · 6 comments · Fixed by #486
Labels
bug Something isn't working

Comments

@madebyfabian
Copy link
Contributor

Environment

Developement/Production OS: MacOS 14.0
Node version: 18.16.1
Package manager: [email protected]
Radix Vue version: 1.0.2
Vue version: 3.0.0
Nuxt version: 3.7.4
CSS framework: [email protected]
Client OS: MacOS 14.0
Browser: Chromium 118

Link to minimal reproduction

https://github.com/radix-vue/radix-vue/blob/main/packages/radix-vue/src/Menu/MenuLabel.vue#L11

Steps to reproduce

Use the DropdownMenuLabel component in any project.

Describe the bug

I discovered that the DropdownMenuLabel uses the MenuLabel component, which renders as <label>. While the react radix renders it as a div.

radix-vue:
https://github.com/radix-vue/radix-vue/blob/main/packages/radix-vue/src/Menu/MenuLabel.vue#L11

radix:
https://github.com/radix-ui/primitives/blob/main/packages/react/menu/src/Menu.tsx#L594

Expected behavior

Expect to have the same html tag rendered for the same component.

Conext & Screenshots (if applicable)

No response

@madebyfabian madebyfabian added the bug Something isn't working label Oct 30, 2023
@madebyfabian
Copy link
Contributor Author

Let me know if that's a bug, or made intentional.
Happy to create a PR for this :)

@zernonia
Copy link
Member

zernonia commented Oct 31, 2023

Nice catch @madebyfabian ! Let me do some test quickly.. I forgot if it was intentional.

Edit: Yes let's change that to div instead of label. Could you help update the docs as well? 😁

@madebyfabian
Copy link
Contributor Author

PR in #486. I also updated it in docs. Checked if there where anything in the tests at Menu.test.ts that should been updated, but I did not found anything.

@madebyfabian
Copy link
Contributor Author

Should I do the same for the SelectLabel component? I just discovered that this is also a<div> in radix react and a <label> in radix-vue.

@zernonia
Copy link
Member

zernonia commented Nov 1, 2023

You are right about that too @madebyfabian.

However, I realized the Combobox.Label in headlessUI were <label>.

Let's change these SelectLabel, ComboboxLabel to div as well as it doesn't affect the accessbility much. 😁

@madebyfabian
Copy link
Contributor Author

madebyfabian commented Nov 2, 2023

@zernonia I think Combobox.Label in Headless UI is refering to the label of the form element 😄. Were <label> is indeed the correct way.

But with Radix, the SelectLabel refers to this piece: which only indicates the name of the following options group in the dropdown content. And I researched a bit about that, seems that the <label> is the wrong tag in this case, especially because radix-vue already sets the aria-* stuff correctly.
Bildschirmfoto 2023-11-02 um 17 31 27

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants