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] List slots in API documentation #36104

Merged
merged 31 commits into from
Feb 21, 2023

Conversation

hbjORbj
Copy link
Member

@hbjORbj hbjORbj commented Feb 8, 2023

Closes #35791

Changes:

  1. For every Base component, this PR creates ${ComponentName}Slots interface for slots prop (inside the interface, it has the JSDoc default value and description for each slot).
  2. Parses the JSDoc for ${ComponentName}Slots interfaces and updates API JSON files
  3. Create a UI for "Slots" section in API Doc, which displays name, type, default and description of each slot
  4. For every Base component, this PR sets default value for slots prop and add JSDoc default value for slots (hence slots in "Props" section of the API doc now has a default value); UPDATE: Reverted this after realising that setting default values to slots prop brings implementation changes

Preview: https://deploy-preview-36104--material-ui.netlify.app/base/api/select-unstyled/#slots <- SelectUnstyled API Doc

Screenshot 2023-02-16 at 23 03 06

@hbjORbj hbjORbj marked this pull request as draft February 8, 2023 06:29
@hbjORbj hbjORbj self-assigned this Feb 8, 2023
@hbjORbj hbjORbj added docs Improvements or additions to the documentation package: base-ui Specific to @mui/base labels Feb 8, 2023
@hbjORbj hbjORbj added this to the MUI Base stable release milestone Feb 8, 2023
@mui-bot
Copy link

mui-bot commented Feb 8, 2023

Netlify deploy preview

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

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against eb04331

@hbjORbj hbjORbj force-pushed the base/list-slots-docs branch 2 times, most recently from 1138d03 to a3f50f9 Compare February 10, 2023 09:04
@hbjORbj hbjORbj marked this pull request as ready for review February 10, 2023 11:39
Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

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

Great job overall! It should be much clearer to the developers what they can use now. However, we definitely need to work on making JSDocs more comprehensive.

Was creating a separate interface for slots necessary? I don't think it'll ever be used on its own. We already have a ton of types starting with {ComponentName}Unstyled, so if we can avoid adding new ones, that would be great. I understand that it allowed you to reuse lots of code responsible for parsing the classes interface. Perhaps it wouldn't be that difficult to get to the description of {ComponentName}UnstyledProps.slots directly instead. Let me know what you think.

Also, tagging the @mui/docs-infra team for a review.

packages/api-docs-builder/utils/parseSlots.ts Outdated Show resolved Hide resolved
@michaldudak michaldudak requested a review from a team February 14, 2023 16:02
@hbjORbj
Copy link
Member Author

hbjORbj commented Feb 14, 2023

Perhaps it wouldn't be that difficult to get to the description of {ComponentName}UnstyledProps.slots directly instead.

Are you suggesting that we add default value for slots prop like { root: 'span', badge: 'span' } and use this to parse default values of each slot?

@siriwatknp
Copy link
Member

siriwatknp commented Feb 16, 2023

Instead of Type column, should it be class name instead? I think it is more useful because Type will always be ElementType for all slots. cc @michaldudak

Screen Shot 2566-02-16 at 14 37 09

@michaldudak
Copy link
Member

Are you suggesting that we add default value for slots prop like { root: 'span', badge: 'span' } and use this to parse default values of each slot?

Only in JSDoc, though (unless it will cause linting errors saying that the prop must be also initialized in code).

Instead of Type column, should it be class name instead? I think it is more useful because Type will always be ElementType for all slots.

Great idea! We don't have slot class names documented anywhere, so it'll come in handy for sure.

@hbjORbj
Copy link
Member Author

hbjORbj commented Feb 16, 2023

Only in JSDoc, though (unless it will cause linting errors saying that the prop must be also initialized in code).

Yes, the code and the default value declared in JSDoc must match to avoid the errors and to pass all CI tests.

@hbjORbj
Copy link
Member Author

hbjORbj commented Feb 16, 2023

I added to the documentations the slot class names as @siriwatknp suggested.

Screenshot 2023-02-16 at 14 50 01

@michaldudak
Copy link
Member

Looks good! I'll do a more thorough review a bit later.
What do you think about changing the "Global class" title to "Default class"? Soon, these classes will have the possibility of being removed (#35963), so IMO it makes sense to underline they are applied by default.

@hbjORbj
Copy link
Member Author

hbjORbj commented Feb 16, 2023

What do you think about changing the "Global class" title to "Default class"? Soon, these classes will have the possibility of being removed (#35963), so IMO it makes sense to underline they are applied by default.

Yes, it sounds better. On my way.

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.

👍 Awesome

@michaldudak
Copy link
Member

Now that I look at it, the "Default" column feels a bit unclear next to "Default classes":
image

Perhaps changing "Default" to "Default value" could make it easier to undersdtand.

@hbjORbj
Copy link
Member Author

hbjORbj commented Feb 20, 2023

Agree. I renamed it.

@hbjORbj hbjORbj changed the title [docs] List slots in MUI Base components documentation [docs][base] List slots in API documentation Feb 20, 2023
Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

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

I like it!

@hbjORbj hbjORbj merged commit a72a53c into mui:master Feb 21, 2023
siriwatknp pushed a commit to mnajdova/material-ui that referenced this pull request Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation package: base-ui Specific to @mui/base
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[docs] List slots in MUI Base components documentation
4 participants