-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
BoxControl: Auto-generate readme #67284
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
* return ( | ||
* <BoxControl | ||
* values={ values } | ||
* onChange={ ( nextValues ) => setValues( nextValues ) } |
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.
Pet peeve 🙈
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.
Looking great, thanks @mirka! 👍
I think it's mostly a question of a couple of props that I'm not sure we want to omit from the docs. Let me know what you think.
|
||
### `onMouseOut`: `function` |
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.
What's the thing about those props like onMouseOut
and onMouseOver
that we previously documented in the README but we no longer do? Does that signal a gap in types perhaps?
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.
The types are correct, it's just the default behavior of react-docgen-typescript
to suppress element props that are optional and don't have explicit descriptions. I think that makes sense, especially because onMouseOut
and onMouseOver
are highly unlikely to be used in common BoxControl use cases, and their inclusion is simply arbitrary (probably only happened because some weird edge case in Gutenberg required it).
What annoys me though is that there's quirk in the API where onMouseOut
and onMouseOver
should actually just be passed as part of the inputProps
prop. That's basically what happens under the hood anyway. I'll address this in a separate PR — it should just be a change in TypeScript metadata.
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.
Sounds good, thanks for clarifying 👍
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.
Looks good, let's 🚢!
What?
Switch the README for
BoxControl
to an auto-generated one (see also #66035).Why?
To improve accuracy and maintenance cost.
Testing Instructions
npm run docs:components
to verify the changes to the README.