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

[experimentalStyled] Add name and slot options #23964

Merged
merged 11 commits into from Dec 13, 2020

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Dec 11, 2020

This PR aims to resolve #23745 (comment) by providing name and slot as options in the experimentalStyled utility.

Here is the output of both the generated HTML and the React tree.

HTML

Previously:
classes old

Now:
Classes new

React tree

Previously:
component names old dev

Now:
components new name

@mui-pr-bot
Copy link

mui-pr-bot commented Dec 11, 2020

@material-ui/core: parsed: +0.06% , gzip: +0.10%

Details of bundle changes

Generated by 🚫 dangerJS against c37d4f7

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.

This looks like an improvement for the DX of customizations :). I wonder if we can't abstract the API, like:

  • name: string Slider
  • slot: string Rail

to avoid naming mistakes & enforce more consistency. For instance, if slot is Root, apply muiName automatically.

@mnajdova
Copy link
Member Author

mnajdova commented Dec 12, 2020

This looks like an improvement for the DX of customizations :). I wonder if we can't abstract the API, like:

  • name: string Slider
  • slot: string Rail

to avoid naming mistakes & enforce more consistency. For instance, if slot is Root, apply muiName automatically.

Definitely looks better, implemented with 3845a64

@mnajdova mnajdova changed the title [experimentalStyled] Add displayName and className options [experimentalStyled] Add name and slot options Dec 12, 2020
@@ -56,7 +56,7 @@ const overridesResolver = (props, styles) => {
const BadgeRoot = styled(
'span',
{},
{ muiName: 'MuiBadge', overridesResolver },
{ name: 'Badge', slot: 'Root', overridesResolver },
Copy link
Member

Choose a reason for hiding this comment

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

I've been meaning to ask – is "slot" a recognised term? I hadn't come across it before your use, and a quick web search doesn't bring up anything relevant.

Copy link
Member Author

@mnajdova mnajdova Dec 12, 2020

Choose a reason for hiding this comment

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

It’s not something standardized, I’ve just got used to use it for describing different parts of the component. In Vue, slots are used as templates part for creating a layout https://vuejs.org/v2/guide/components-slots.html Here is an example:

<div class="container">
  <header>
    <!-- We want header content here -->
    <slot name="header"></slot>
  </header>
  <main>
    <!-- We want main content here -->
    <slot name="body"></slot>
  </main>
  <footer>
    <!-- We want footer content here -->
    <slot name="footer"></slot>
  </footer>
</div>

I don’t think react has a similar notion for it, but I think we can just use the same term. Open for other options around naming here :)

Copy link
Member

Choose a reason for hiding this comment

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

I was influenced by the usage of the slot terminology by Vue and Web Components when making the initial proposal. The first idea that came to mind was component but it seemed to be overloaded. Hence slot as the second option.

Any other idea?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

@mnajdova mnajdova Dec 13, 2020

Choose a reason for hiding this comment

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

part is the other term I had in mind too, but slot sound better to me 🤷‍♀️

Copy link
Member

Choose a reason for hiding this comment

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

svelte has slots as well.

Copy link
Member

Choose a reason for hiding this comment

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

I'm struggling to see how "slot" as used here relates to slots in Vue and Svelte, where it appears to be a placeholder for children equivalent to {props.children} in React. A slot is something that you put things into. That doesn't seem to be the case here, unless I'm misunderstanding.

Copy link
Member

Choose a reason for hiding this comment

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

@mbrookes each components' prop key is a slot.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, thanks.

@mnajdova mnajdova merged commit 3506dda into mui:next Dec 13, 2020
@zannager zannager added package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. component: badge This is the name of the generic UI component, not the React module! labels Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: badge This is the name of the generic UI component, not the React module! package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants