-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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-infra] New Component API design followup #38183
[docs-infra] New Component API design followup #38183
Conversation
Netlify deploy previewhttps://deploy-preview-38183--material-ui.netlify.app/ Bundle size report |
@@ -88,7 +88,7 @@ export default function PropertiesTable(props) { | |||
.filter((key) => propData.additionalInfo?.[key]) | |||
.map((key) => ( | |||
<p | |||
className="prop-list-additional-info" | |||
className="prop-list-description" |
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.
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.
Would it be possible to add this content in an info
callout?
Additionally, it'd be great to render content wrapped in backticks as a code
block!
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.
Nit: how about putting "the" in the link? Like:
See the
sx
page for more details.
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 about the info callout. It's the kind of text you read once, then you can ignore it.
It's for new users encountering this kind of props for the first time, they got the link to the docs just next to the description they did not understand.
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.
Uhm... alright, I see your point ⎯ adding the callout would put too much unnecessary emphasis.
Another copy suggestion to not repeat sx
:
See the documentation for more details.
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.
Yes, those wordings did not get attention for a long time. They could get some reviews from @samuelsycamore all together at once
Here you can find all the API descriptions added automatically
typeDescriptions[name] = renderMarkdownInline(description); | ||
}, | ||
); | ||
(signatureArgs || []).concat(signatureReturn || []).forEach(({ name, description }) => { |
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.
Just a simplification
@@ -563,7 +562,8 @@ const attachPropsTable = (reactApi: ReactApi) => { | |||
deprecationInfo: | |||
renderMarkdownInline(deprecation?.groups?.info || '').trim() || undefined, | |||
signature, | |||
...(Object.keys(additionalPropsInfo).length === 0 ? {} : { additionalPropsInfo }), | |||
additionalInfo: |
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.
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.
Similarly here, it'd be great to have these in an info
callout. Also, I think I'd probably put it below the "Default" text ⎯ it's complementary & generic info whereas "Type" & "Default" are the prop's main pieces of content.
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.
About the text itself, how about:
See the Slots API section below for more details.
Then, similar to the above, having "the Slots API section" as the link!
const componentProps: ReactApi['propsTable'] = _.fromPairs( | ||
Object.entries(reactApi.props!).map(([propName, propDescriptor]) => { | ||
Object.entries(reactApi.props!).map(([propName, propDescriptor]): Pair => { |
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.
Improved typing to prevent typos like additionalPropsInfo
below
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.
Thanks for the clear PR 👌
Co-authored-by: alexandre <[email protected]>
Co-authored-by: alexandre <[email protected]>
Follow-up for #37405
I've added an explanatory comment for each change I did, all other changes are generated by the
docs:api
script.Example page: https://deploy-preview-38183--material-ui.netlify.app/joy-ui/api/alert/
TODO: