-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[docs] Move more prop docs into IntelliSense #20298
[docs] Move more prop docs into IntelliSense #20298
Conversation
Details of bundle changes.Comparing: c16f941...13bef15 Details of page changes
|
With or without the unmerged PRs? |
This PR only requires merceyz/typescript-to-proptypes#7 |
Released, i'll take a look at your PRs this weekend |
@@ -279,12 +283,12 @@ Dialog.propTypes = { | |||
/** | |||
* Dialog children, usually the included sub-components. | |||
*/ | |||
children: PropTypes.node.isRequired, | |||
children: PropTypes.node, |
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.
Not sure what the point is making a node
required. One could still omit content by passing false
. TypeScript allowed undefined | null
since the beginning.
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 was meant to tell users that instead of
<Dialog>{condition ? <B /> : null}</Dialog>
they might prefer
{condition ? <Dialog><B /></Dialog> : null}
But no strong preference
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.
<Dialog>{condition ? <B /> : null}</Dialog>
This is how I prefer to write conditional rendering as well but I see very many people defaulting to shorthand <Dialog>{condition && <B />}</Dialog>
which does pass PropTypes.node.isRequired
I wouldn't mind adding a warning with a rationale why one pattern should be preferred but then it needs a rationale, an actionable hint and needs to be applied consistently.
disabled?: boolean; | ||
IconComponent?: React.ElementType; | ||
inputRef?: ( | ||
ref: HTMLSelectElement | { node: HTMLInputElement; value: NativeSelectInputProps['value'] }, |
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.
Could not find how this is implemented.
@@ -26,16 +26,21 @@ export const styles = { | |||
const Step = React.forwardRef(function Step(props, ref) { | |||
const { | |||
active = false, | |||
// eslint-disable-next-line react/prop-types |
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.
private props are rare enough so that we don't need to handle them. If missing validation becomes an issue when debugging we can revisit. But these components haven't been touched in a long time.
@@ -279,12 +283,12 @@ Dialog.propTypes = { | |||
/** | |||
* Dialog children, usually the included sub-components. | |||
*/ | |||
children: PropTypes.node.isRequired, | |||
children: PropTypes.node, |
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 was meant to tell users that instead of
<Dialog>{condition ? <B /> : null}</Dialog>
they might prefer
{condition ? <Dialog><B /></Dialog> : null}
But no strong preference
Includes the following fixes to TypeScript types:
NativeSelect
input
only allowsReactElements
not just anyReactNode
NativeSelect
inputProps
now allows all<select>
attributesBlocked until new release of typescript-to-proptypes