Skip to content

Update component docs (Header, Radio, Checkbox, Heading, Label, LabelGroup) - #1708

Merged
rezrah merged 17 commits into
mainfrom
docs/convert-prop-tables-pt1
Dec 20, 2021
Merged

Update component docs (Header, Radio, Checkbox, Heading, Label, LabelGroup)#1708
rezrah merged 17 commits into
mainfrom
docs/convert-prop-tables-pt1

Conversation

@rezrah

@rezrahrezrah commented Dec 9, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

@changeset-bot

changeset-botBot commented Dec 9, 2021•
edited
Loading

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: ff10c7c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@rezrahrezrah added the skip changeset This change does not need a changelog label Dec 9, 2021
@github-actions

github-actionsBot commented Dec 9, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
dist/browser.esm.js58.92 KB (0%)
dist/browser.umd.js59.25 KB (0%)

@rezrahrezrah changed the title Update component docs (Header, Radio, Checkbox)Update component docs (Header, Radio, Checkbox, Heading)Dec 9, 2021
@rezrahrezrah changed the title Update component docs (Header, Radio, Checkbox, Heading)Update component docs (Header, Radio, Checkbox, Heading, Label, LabelGroup)Dec 9, 2021
@rezrahrezrah mentioned this pull request Dec 9, 2021
Closed
59 tasks
@rezrah
rezrah marked this pull request as ready for review December 9, 2021 17:33
@rezrah
rezrah requested review from a team and siddharthkpDecember 9, 2021 17:33
@rezrah
rezrah requested a review from colebemisDecember 9, 2021 17:39
colebemis
colebemis reviewed Dec 9, 2021
View reviewed changes
Comment threaddocs/content/Checkbox.mdx
colebemis
colebemis reviewed Dec 9, 2021
View reviewed changes
Comment threaddocs/content/Checkbox.mdx
/>
<PropsTableRow
name="disabled"
type="boolean"

@colebemiscolebemisDec 9, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this have a default value?

@rezrahrezrahDec 10, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It's an optional prop, so no. Or are you setting undefined explicitly?

@colebemiscolebemisDec 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it make sense to say this defaults tofalse?

@rezrahrezrahDec 10, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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?

colebemis
colebemis reviewed Dec 9, 2021
View reviewed changes
Comment threaddocs/content/Checkbox.mdx Outdated
colebemis
colebemis reviewed Dec 9, 2021
View reviewed changes
Comment threaddocs/content/Checkbox.mdx Outdated
colebemis
colebemis reviewed Dec 9, 2021
View reviewed changes
Comment threaddocs/content/Checkbox.mdx
## Props

Native `<input>` attributes are forwarded to the underlying React `input` component and are not listed below.
<PropsTable>

@colebemiscolebemisDec 9, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The heading of prop tables is usually the name of the component:

Suggested change
<PropsTable>
### Checkbox
<PropsTable>

@rezrahrezrahDec 10, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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?

colebemis
colebemis reviewed Dec 9, 2021
View reviewed changes
Comment threaddocs/content/Checkbox.mdx Outdated
colebemis
colebemis reviewed Dec 9, 2021
View reviewed changes
Comment threaddocs/content/Header.mdx Outdated
colebemis
colebemis reviewed Dec 9, 2021
View reviewed changes
Comment threaddocs/content/Header.mdx Outdated
</PropsTable>

### Header.Item
### `Header.Item`

@colebemiscolebemisDec 9, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
### `Header.Item`
### Header.Item

@rezrahrezrahDec 10, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Updated. But I do think the syntax-like content makes this quite an ugly heading

colebemis
colebemis reviewed Dec 9, 2021
View reviewed changes
Comment threaddocs/content/Header.mdx Outdated
</PropsTable>

### Header.Link
### `Header.Link`

@colebemiscolebemisDec 9, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
### `Header.Link`
### Header.Link

@rezrahrezrahDec 10, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Swapped it back, but it feels pretty ugly as the other headings include object syntax.

colebemis
colebemis reviewed Dec 9, 2021
View reviewed changes
Comment threaddocs/content/Header.mdx Outdated
<PropsTable>
<PropsTableRow name="href" type="string" description="URL to be used for the Link" />
<PropsTableAsRow defaultElementType="a" />
<PropsTableSxRow />

@colebemiscolebemisDec 9, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this component also pass props down to the <a> element?

@rezrahrezrahDec 10, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Looks like it does, updated with base props. Is that more suitable? Maybe not as there isn't any ref forwarding 🤔

@rezrahrezrahDec 10, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Update: I've put a guard on refType so that row doesn't appear.

colebemis
colebemis reviewed Dec 9, 2021
View reviewed changes
Comment threaddocs/content/Header.mdx
colebemis
colebemis reviewed Dec 9, 2021
View reviewed changes
Comment threaddocs/content/Label.mdx Outdated
colebemis
colebemis reviewed Dec 9, 2021
View reviewed changes
Comment threaddocs/content/Radio.mdx Outdated
colebemis
colebemis reviewed Dec 9, 2021
View reviewed changes
Comment threaddocs/content/Radio.mdx Outdated
colebemis
colebemis reviewed Dec 9, 2021
View reviewed changes
Comment threaddocs/content/Radio.mdx Outdated
colebemis
colebemis reviewed Dec 9, 2021
View reviewed changes
Comment threaddocs/content/LabelGroup.mdx
colebemis
colebemis reviewed Dec 9, 2021
View reviewed changes
Comment threaddocs/content/Heading.mdx
colebemis
colebemis reviewed Dec 9, 2021
View reviewed changes
Comment threaddocs/content/Heading.mdx
colebemis
colebemis reviewed Dec 9, 2021
View reviewed changes
Comment threaddocs/content/Label.mdx
colebemis
colebemis reviewed Dec 9, 2021
View reviewed changes
Comment threaddocs/content/Header.mdx
rezrah
rezrah commented Dec 10, 2021
View reviewed changes
Comment threaddocs/content/Label.mdx
<ComponentChecklist
items={{
propsDocumented: true,
noUnnecessaryDeps: false,

@rezrahrezrahDec 10, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This component seems to expose styled-system, which I presume is an unnecessary dep?

@colebemiscolebemisDec 17, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since styled-system is required by the whole library, I don't think I'd consider it an unnecessary dep

rezrah
rezrah commented Dec 10, 2021
View reviewed changes
Comment threaddocs/content/Label.mdx
noUnnecessaryDeps: false,
adaptsToThemes: true,
adaptsToScreenSizes: false,
fullTestCoverage: false,

@rezrahrezrahDec 10, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

75% on the some conditional behaviour

rezrah
rezrah commented Dec 10, 2021
View reviewed changes
Comment threaddocs/content/Label.mdx
propsDocumented: true,
noUnnecessaryDeps: false,
adaptsToThemes: true,
adaptsToScreenSizes: false,

@rezrahrezrahDec 10, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

rezrah
rezrah commented Dec 10, 2021
View reviewed changes
Comment threaddocs/src/props-table.js
colebemis
colebemis approved these changes Dec 17, 2021
View reviewed changes

@colebemiscolebemis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should be good to go after the merge conflicts are addressed 👍

@rezrah
rezrah merged commit 2b3744e into mainDec 20, 2021
@rezrah
rezrah deleted the docs/convert-prop-tables-pt1 branch December 20, 2021 14:42
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@siddharthkpsiddharthkpAwaiting requested review from siddharthkp
1 more reviewer
@colebemiscolebemiscolebemis approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

skip changesetThis change does not need a changelog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rezrah@colebemis