-
-
Notifications
You must be signed in to change notification settings - Fork 356
[field][docs] Update Field.Item related docs
#3809
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
[field][docs] Update Field.Item related docs
#3809
Conversation
commit: |
Bundle size report
Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Greptile SummaryThis PR fixes a critical documentation bug where
The updated documentation now matches the correct usage pattern shown in the codebase tests, where Confidence Score: 5/5
Important Files Changed
|
| 2. **Individual label**: Place each individual checkbox in `<Field.Item>`, then label it using an enclosing `<Field.Label>`. Enclosing labels ensure that clicking on any gaps between the label and checkbox still toggles it. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why (implementation wise) this is necessary? Is there a way to avoid it for the enclosing case?
All the other component docs about labeling would also need to be updated here as they omit <Field.Item>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC it's because we changed from <button> to spans so even for enclosing labels htmlFor needs to be set explicitly
Is there a way to avoid it for the enclosing case?
Could possibly make Field.Label render Field.Item internally when necessary? 🤔
All the other component docs about labeling would also need to be updated here
This is only relevant for checkbox group and radio group though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC it's because we changed from to spans so even for enclosing labels htmlFor needs to be set explicitly
What does the htmlFor do though when enclosed? IIRC the input becomes the label focus target when enclosed, which redirects focus to the <span>, so it works anyway. And when focus is on the <span>, the label text is still announced to screen readers (as it works with native <label> as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems intentional since all the tests were updated to have FieldItem in all cases, perhaps it can be omitted for enclosing FieldLabels as long as it doesn't trigger any chrome errors
Update: it works but does indeed cause duplicate id errors in Chrome #3815 (comment) @atomiks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, let's update the relevant docs to use <Field.Item> then
Field.Item related docsField.Item related docs
Closes #3807