-
-
Notifications
You must be signed in to change notification settings - Fork 114
[docs] Mark required props in API docs #1756
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
base: master
Are you sure you want to change the base?
[docs] Mark required props in API docs #1756
Conversation
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Why does this need to be documented? It becomes clear pretty quickly in the IDE, no? I'm not a fan of adding superfluous content to the actual prop name in the docs, it could be misleading. |
Then why document any props at all if they're visible in the IDE? Also, did you see this comment? #1694 (comment) |
Well because not all props are equal in this regard. I'm not saying nothing should change here, I'm just saying that making the prop names inaccurate in the table is not the right solution. |
I assume you're referring to the asterisk after the prop names. From what I've seen, it's a common convention — for example:
Do you have a different suggestion in mind? |
I think the asterisk is ok but it needs to be in a |
@atomiks Made the changes. |
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.
This change looks fine to me 👍
cc: @colmtuite @mnajdova @michaldudak
Perhaps title="required"
or something else is required for screen readers.
Closes #1694 (See #1694 (comment))
Preview:
Radio.Root
- https://deploy-preview-1756--base-ui.netlify.app/react/components/radio#rootMenu.RadioItem
- https://deploy-preview-1756--base-ui.netlify.app/react/components/menu#radio-item