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

[Joy] Support slots/slotsProps for every component (components with only root slot too) #36540

Merged
merged 21 commits into from
Apr 10, 2023

Conversation

hbjORbj
Copy link
Member

@hbjORbj hbjORbj commented Mar 16, 2023

Follow-up on (4) of #36490 (comment)

Changes

  • Joy components that only have root slot also support slots/slotProps
  • Consequently, every Joy component's API doc now has "Slots" section.

Preview: e.g., https://deploy-preview-36540--material-ui.netlify.app/joy-ui/api/card/#slots

⚠️ This PR must be merged after #36599 is merged.

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

mui-bot commented Mar 16, 2023

Netlify deploy preview

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

@mui/joy: parsed: +0.44% , gzip: +0.16%

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against 11f748a

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Follow-up on (4) of #36490 (comment)

I'm not sure about this direction. When I land on the page, I assume that I can do slots={{ root }} but it's not the case. What I was wondering there is about the class names. I wonder if there is a way (design-wise) to communicate this. Maybe it's about having more columns. As a developer, I would want to understand, that I can use a class name, a theme component style override but instead of slots.root => component.


export type CardSlot = 'root';

export interface CardSlots {
Copy link
Member

Choose a reason for hiding this comment

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

I assume that we can't export any of these types since there is no slots prop?

Suggested change
export interface CardSlots {

Copy link
Member

Choose a reason for hiding this comment

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

@hbjORbj Is it necessary to export these interfaces (including other components)?

Copy link
Member Author

Choose a reason for hiding this comment

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

For other components, yes, we need to export this to read the JSDoc and generated API docs. More specifically, parseSlots function needs it. https://github.com/mui/material-ui/blob/master/packages/api-docs-builder/utils/parseSlots.ts

@siriwatknp
Copy link
Member

Follow-up on (4) of #36490 (comment)

I'm not sure about this direction. When I land on the page, I assume that I can do slots={{ root }} but it's not the case. What I was wondering there is about the class names. I wonder if there is a way (design-wise) to communicate this. Maybe it's about having more columns. As a developer, I would want to understand, that I can use a class name, a theme component style override but instead of slots.root => component.

If my understanding is correct, what you are asking should be done in a separate PR?

@hbjORbj
Copy link
Member Author

hbjORbj commented Mar 20, 2023

@oliviertassinari @siriwatknp During the Friday backlog grooming session, I brought up this topic (displaying classnames for Joy/Base in API docs), and Marija/Michal/I agreed on creating a new section where we list the classnames except those already displayed in "Default class" column under "Slots" section. I am preparing a PR for this. After I create this PR, we can discuss again to make adjustments.

@oliviertassinari
Copy link
Member

I guess the PR in question is #36589. It's connected, but these seems different problems: modifier class names vs. class names of components without slots prop.

@hbjORbj
Copy link
Member Author

hbjORbj commented Mar 23, 2023

I agree that Joy users might expect slots/slotProps to work for every component regardless of the number of slots. They won't check the API reference to know if the component supports these props or not, but rather they just go for these props because they are used to using them. Even if they realise that these props aren't supported, they will be confused because they don't know why they aren't.

I think we have first clearly communicate that component !== slots.root in order to explain why we find it unnecessary to support slots/slotProps for components with only root slot.

cc @siriwatknp

@siriwatknp
Copy link
Member

I agree that Joy users might expect slots/slotProps to work for every component regardless of the number of slots. They won't check the API reference to know if the component supports these props or not, but rather they just go for these props because they are used to using them. Even if they realise that these props aren't supported, they will be confused because they don't know why they aren't.

👍 All in. All components should support component, slots, and slotProps (except CssBaseline). I found a valid use case for using slots for small component like MenuButton (single DOM) when I want to replace the styles with my custom component but preserve the functionality of the Menu Button.

<MenuButton slots={{ root: IconButton }}>
  <Icon />
</MenuButton>

I think we have first clearly communicate that component !== slots.root in order to explain why we find it unnecessary to support slots/slotProps for components with only root slot.

That will be done by #34990

@hbjORbj hbjORbj changed the title [docs][joy] Add Slots section for Card component in API doc [Joy] Support slots/slotsProps for components with only root slot too Mar 24, 2023
@hbjORbj
Copy link
Member Author

hbjORbj commented Mar 24, 2023

I made a change so that Joy components that only have root slot also support slots/slotProps. Consequently, every Joy component's API doc now has "Slots" section.

@hbjORbj hbjORbj force-pushed the card/api-doc branch 2 times, most recently from f2d3781 to 7a6cccd Compare March 26, 2023 21:25
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 27, 2023
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.

@hbjORbj I think some components like Card need to import and use useSlot because it currently does not accept slots or slotProps.

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

hbjORbj commented Mar 27, 2023

@siriwatknp You are right. My bad for missing that. Doing it now!

UPDATE: All ready, I think.

@hbjORbj hbjORbj removed the request for review from oliviertassinari March 28, 2023 02:52
@hbjORbj hbjORbj changed the title [Joy] Support slots/slotsProps for components with only root slot too [Joy] Support slots/slotsProps for every component (components with only root slot too) Mar 28, 2023
@hbjORbj hbjORbj requested a review from siriwatknp March 28, 2023 16:35
open,
disablePortal,
keepMounted,
as: PopperUnstyled,
Copy link
Member Author

Choose a reason for hiding this comment

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

This refactoring of Menu is part of #36599. #36599 must be merged before this PR is merged.

@hbjORbj
Copy link
Member Author

hbjORbj commented Mar 31, 2023

@siriwatknp Reminder

@hbjORbj
Copy link
Member Author

hbjORbj commented Apr 9, 2023

@siriwatknp

I think we can merge this? I rebased this branch on top of #36599

Here's a filter of relevant commits for easier review: https://github.com/mui/material-ui/pull/36540/files/f7d03cd84301166c96df417ed8fc47e356c1c85a..01cb82b550c119222d5babde2c90dca12d5b3d64

UPDATE: Merged #36599. This PR is rebased. 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 10, 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 10, 2023
@hbjORbj hbjORbj merged commit 0403162 into mui:master Apr 10, 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: joy-ui Specific to @mui/joy
Projects
Status: Recently completed
Development

Successfully merging this pull request may close these issues.

None yet

4 participants