Skip to content
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][base & joy] Display "Classes" Section in API docs #36589

Merged
merged 21 commits into from
Apr 7, 2023

Conversation

hbjORbj
Copy link
Member

@hbjORbj hbjORbj commented Mar 20, 2023

Before:

After:

@hbjORbj hbjORbj self-assigned this Mar 20, 2023
@hbjORbj hbjORbj added docs Improvements or additions to the documentation package: base-ui Specific to @mui/base package: joy-ui Specific to @mui/joy labels Mar 20, 2023
@mui-bot
Copy link

mui-bot commented Mar 20, 2023

Netlify deploy preview

https://deploy-preview-36589--material-ui.netlify.app/

Bundle size report

No bundle size changes

Generated by 🚫 dangerJS against 446a37b

@oliviertassinari oliviertassinari added the design This is about UI or UX design, please involve a designer label Mar 21, 2023
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Thanks Benny, few comments.

  1. Should we name the section State classes? All other classes are included in the slots section
  2. Wait for the descriptions to be added before merging this
  3. Make the table same width as the other tables on the page

We will have conflicts with the tabs API PR, this is why I proposed to wait for that one to be merged first, but hopefully we can resolve the merge conflicts successfully.

Would be great if you can add tests for the changes introduced

@hbjORbj
Copy link
Member Author

hbjORbj commented Mar 22, 2023

@mnajdova

  1. Should we name the section State classes? All other classes are included in the slots section

Agree. Done.

  1. Wait for the descriptions to be added before merging this

Sure👌

  1. Make the table same width as the other tables on the page

Done.

We will have conflicts with the tabs API PR, this is why I proposed to wait for that one to be merged first, but hopefully we can resolve the merge conflicts successfully.

Got it👌

Would be great if you can add tests for the changes introduced

Can you elaborate please? Tests for the function parseSlotsAndClasses, or do you mean something else?

},
"name": "MuiSlider"
},
"styles": { "classes": [], "globalClasses": {}, "name": "MuiSlider" },
Copy link
Member Author

@hbjORbj hbjORbj Mar 22, 2023

Choose a reason for hiding this comment

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

Joy Slider apparently had a "CSS" Section while all other Joy components didn't. Now we rather have a "State classes" section.

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 22, 2023

The concept is interesting 👍.

The only information that I can think of that is missing is which slot is the class name available on, but it's likely overkill to solve this problem.

@siriwatknp
Copy link
Member

siriwatknp commented Mar 24, 2023

I find Rule name not useful, would this be simpler? and more explicit?

image

Or the list of the class names could be a table with description of each class name (that's even better).

@hbjORbj
Copy link
Member Author

hbjORbj commented Mar 24, 2023

  • Added the description @siriwatknp suggested under "State classes" heading
  • Removed "Rule name" column
  • We now have 2 columns: "Global class" and "Description"
  • Changed the description of Joy component classes from "Styles applied to..." to "Class name applied to..." for consistency

@hbjORbj
Copy link
Member Author

hbjORbj commented Mar 26, 2023

The only information that I can think of that is missing is which slot is the class name available on, but it's likely overkill to solve this problem. @oliviertassinari

Doesn't the "Slots' table already achieve this? The slots with non-empty value cell in "Default class" column have matching class names while those with empty cell do not. Or I may have misunderstood your point.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 31, 2023
@hbjORbj
Copy link
Member Author

hbjORbj commented Mar 31, 2023

@siriwatknp @oliviertassinari Review reminder

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 31, 2023
@hbjORbj
Copy link
Member Author

hbjORbj commented Apr 4, 2023

Been waiting for review quite a while 🥲

Rebased on master and applied the changes to ComponentsApiContent as well. Should be ready for merge. cc @mnajdova

@siriwatknp
Copy link
Member

@hbjORbj Push some fixes:

  • rename State classes to Class names to be consistent with React
  • add classnames to TOC (it was missing)
  • sort the class names so that state class names come up first.

@hbjORbj
Copy link
Member Author

hbjORbj commented Apr 5, 2023

Thanks. I think this is ready for merge.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 5, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 5, 2023
@hbjORbj hbjORbj requested review from samuelsycamore and removed request for oliviertassinari, mnajdova and michaldudak April 5, 2023 13:19
@samuelsycamore
Copy link
Member

This is looking great! I was just randomly clicking through different components and I noticed that there's no info for the Joy UI Card component. Is there a reason why this one was skipped? If not, we should comb through these one more time to make sure all pages are included.

@hbjORbj
Copy link
Member Author

hbjORbj commented Apr 5, 2023

@samuelsycamore Thanks for the review. This currently excludes components that only have root slot. If we merge #36540 and rebase this PR on top, the problem will be fixed.

@samuelsycamore
Copy link
Member

Ah ok thanks for clarifying @hbjORbj ! In that case I think this is good to go.

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 Thanks for working on this!

@mnajdova
Copy link
Member

mnajdova commented Apr 6, 2023

We are missing the CSS classes title int he table of content in: https://deploy-preview-36589--material-ui.netlify.app/base/react-switch/components-api/

@hbjORbj
Copy link
Member Author

hbjORbj commented Apr 7, 2023

@mnajdova Thanks for the catch. Fixed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design This is about UI or UX design, please involve a designer docs Improvements or additions to the documentation package: base-ui Specific to @mui/base package: joy-ui Specific to @mui/joy
Projects
Status: Recently completed
Development

Successfully merging this pull request may close these issues.

6 participants