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

[Slider] Extract slots as standalone components #22893

Merged
merged 30 commits into from
Nov 14, 2020

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Oct 5, 2020

This PR transforms the slots inside the Slider as standalone styled components. This will allow us to support back the classes prop on this component. Plaground for testing the classes prop here -https://codesandbox.io/s/continuousslider-material-demo-forked-40y7u?file=/demo.js

@mui-pr-bot
Copy link

mui-pr-bot commented Oct 5, 2020

@material-ui/lab: parsed: +0.21% , gzip: +0.17%

Details of bundle changes

Generated by 🚫 dangerJS against 1007f6f

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 5, 2020
@mnajdova
Copy link
Member Author

mnajdova commented Oct 7, 2020

Closing, as it was done for experiment only

@mnajdova mnajdova reopened this Nov 12, 2020
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 12, 2020
@mnajdova
Copy link
Member Author

I'll need to update the buildAPI script again to extract info from the proptypes classes prop, currently it depends on the component.styles which is something we are adding for the components created with withStyles. However I would like first to see whether we agree with this approach, before spending too much time on it.

@mnajdova
Copy link
Member Author

@oliviertassinari oliviertassinari added component: slider This is the name of the generic UI component, not the React module! and removed performance labels Nov 12, 2020
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 trade-off is tough. Initially, we have started to work in this pull request in order to evaluate the performance implication. We shared the results in #22342 (comment), then started to engage with the community about it. To summarize we have:

pros for dropping classes:

  1. the API was meant for JSS, there aren't any real use cases outside of it. Assuming JSS is here to fade, the prop is, long term, extra weight with no value.
    jQuery UI added the prop and justified it for global .css file switch. Same rationale in react-autosuggest. Does it really make sense?
  2. each extra styled component slows things down. For instance, take the Button, we will need one for the root and one for the label. We have measured a 20% performance decrease with the slider ([RFC] v5 styling solution 💅 #22342 (comment)). The slider has 7 intermediary elements. For the data grid, it's highly unlikely that we can accept this degradation, at least, for the grid viewport.
  3. early feedback from the community on the discussion were: -20% of performance? drop it.

pros for keeping classes:

  1. makes the migration from v4 to v5 easier. Without, developers that customize the components will need to rewrite.
  2. specificity if flatten usually, it means simpler customizations.
  3. When there are multiple intermediary components. emotion CSS injection priority only works if the styles are applied to the same component. If you target a different one, you need to increase specificity to win. Having a specificity of one by default help. I think that it also helps SASS users as they don't need to nest selectors, they can write them flat.

Happy to go in this direction

};

return utilityClasses;
};

const isComponent = (element) => typeof element !== 'string';
const isHostComponent = (element) => typeof element === 'string';
Copy link
Member

Choose a reason for hiding this comment

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

A note for the second component we will migrate, we might want to abstract this helper, we will need it everywhere

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, makes sense, probably there will be other things that can be abstracted, but for the thing I wasn't sure I just added them inline for now :)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, better until we know for sure :)

Copy link
Member

Choose a reason for hiding this comment

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

This discussion reminds me of something 🤔

(something wet... 😉 )

Copy link
Member Author

Choose a reason for hiding this comment

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

It was too fresh not to use it 😅

@mnajdova mnajdova merged commit 0d2db82 into mui:next Nov 14, 2020
@mnajdova
Copy link
Member Author

I am merging this one so I can update #23308 I can address any additional comments there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: slider This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants