Update component docs (Header, Radio, Checkbox, Heading, Label, LabelGroup) #1708
Conversation
|
size-limit report 📦
|
rezrah
changed the title
rezrah
changed the title
| /> | ||
| <PropsTableRow | ||
| name="disabled" | ||
| type="boolean" |
There was a problem hiding this comment.
Does this have a default value?
There was a problem hiding this comment.
It's an optional prop, so no. Or are you setting undefined explicitly?
There was a problem hiding this comment.
Would it make sense to say this defaults tofalse?
There was a problem hiding this comment.
I think that's misrepresenting the data type, as false !== undefined. I think literally undefined would make sense, or to provide a way to clearly indicate it's optional the same way you do with required?
|
|
||
| Native `<input>` attributes are forwarded to the underlying React `input` component and are not listed below. | ||
| <PropsTable> |
There was a problem hiding this comment.
The heading of prop tables is usually the name of the component:
| <PropsTable> | |
| ### Checkbox | |
| <PropsTable> |
There was a problem hiding this comment.
The example heading in the Issue description here shows it as Props, which feels sensible given the content it describes... or am I misunderstanding this? What was the reasoning behind repeating the component name here?
| <ComponentChecklist | ||
| items={{ | ||
| propsDocumented: true, | ||
| noUnnecessaryDeps: false, |
There was a problem hiding this comment.
This component seems to expose styled-system, which I presume is an unnecessary dep?
There was a problem hiding this comment.
Since styled-system is required by the whole library, I don't think I'd consider it an unnecessary dep
| noUnnecessaryDeps: false, | ||
| adaptsToThemes: true, | ||
| adaptsToScreenSizes: false, | ||
| fullTestCoverage: false, |
There was a problem hiding this comment.
75% on the some conditional behaviour
| propsDocumented: true, | ||
| noUnnecessaryDeps: false, | ||
| adaptsToThemes: true, | ||
| adaptsToScreenSizes: false, |
There was a problem hiding this comment.
Unsure about this one. Arguably it could have some responsive behaviour to ensure size variants respect the constraints of the current screen and medium, so that the user doesn't have to do this themselves.
Part of #1701
Previews
Header
Radio
Checkbox
Heading
Label
LabelGroup