-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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] Update buildAPI script to handle the "styled" components #23370
[docs] Update buildAPI script to handle the "styled" components #23370
Conversation
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.
On a related note, the changes make me wonder if we need a section for the components
prop, to describe what each component do, or maybe an interactive playground that change the color background could do the trict.
/** Styles applied to the root element. */ | ||
| 'root' |
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.
This won't do anything. JSDOC for literals is ignored: microsoft/TypeScript#25499.
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.
Aha, now I understand why I didn't have these on the node directly, so I had to iterate all leadingComments
and see which one is right before the specific classKey
- see #23370 (comment) I am open to other ideas of how to do this.
On hold pending #23214 |
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
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.
We should use a more approachable format for this documentation. Encoding it in types where it isn't useful at all is not helpful long-term.
It make sense to encode in types if it has a direct impact and a first-class API. Extracting it manually is too brittle.
Do you have some suggestion for better alternative? We could do it manually, but I don't think that is a better alternative at this moment. I understand that adding comments on the union type is not he best option, but I don't have better ideas to be honest. |
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.
I think we should just fork it entirely. The "CSS" section doesn't make much sense for the unstyled classnames:
- rule name is irrelevant
classes
object is not implemented- mention of implementation is not helpful since the definition of these rules is spread throughout the implementation.
It might be impossible to extract statically without using actual types.
It seems to me a minimal approach at first is better suited i.e. don't abstract before we know there's an abstraction:
- classnames with description in JSON
- separate function for generating the CSS section
Over time we move more and more components to that new format and during that time we'll see what makes more sense. Abstracting it just for one component might not pay off long term.
@eps1lon what do you think of these changes - mnajdova#12 |
@eps1lon done let's do final round of reviews to unblock #23308 Note: You mention that we don't need the const theme = createMuiTheme({
components: {
MuiSlider: {
styleOverrides: {
colorPrimary: { // one key from the `classes`
// some styles
},
},
},
},
}); |
What does the value under the class key add? It seems like redundant information that can be reconstructed. |
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.
Coninuing mnajdova#12 (comment):
Where am I mentioning the classes prop?
The docs are mentioning it:
With a rule name of the classes object prop.
-- https://deploy-preview-23370--material-ui.netlify.app/api/slider-styled/#css
The docs also still mention the implementation which is no longer helpful (and never really was):
If that's not sufficient, you can check the implementation of the component for more detail.
I still recommend completely forking the approach. This advise has been ignored while no alternative has been provided. I don't care whether you follow my proposal or someone else's. But the problems mentioned in a review should be resolved in some shape or form.
Alright will change the generated markdown. I wanted to keep the PR focused on how the information is extracted, not the actual content, that’s why I couldn’t understand where I am mentioning the classes prop. Will change the markdown generated together with these changes so the changes are complete. |
These keys are still used under the
|
I've updated the description for the CSS section of the markdown, hopefully it is more clear now when it comes to "styled" components |
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. Just need to clarify what the runtime change is for.
Co-authored-by: Matt <github@nospam.33m.co>
This PR prepares necessary changes for generating the
CSS
andComponent name
sections in the docs file for the components that does not usewithStyles
and does not contain theclasses
prop. It extract this information based on the JSON data available atdocs/data/component-name.json
.