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] Create unstyled version and migrate to emotion & styled-components #22435

Merged
merged 135 commits into from
Sep 23, 2020

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Sep 1, 2020

This pull request is meant to figure out most of the tradeoffs required to deliver the migration to emotion. The Slider component is used as a test case. Things done in the PR:

  • Created SliderUnstyled component, which basically contains all business logic of the component, plus generation of utility classes that developers can used for styling it.
  • Created Slider component, which is basically adding the material design styles on top of the SliderUnstyled component, by creating a styled component for the root of the Slider.
  • Created muiStyled factory that is wrapper around the styled utilities supported that adds on top of the original styles the overrides and variants defined in the theme.
  • Created two packages for the style engine
  1. @material-ui/styled-engine - simple wrapper package around @emotion/styled - used by default in the core
  2. @material-ui/styled-engine-sc - simple wrapper package around styled-components - intended for the developers that use styled-components. Developers should configure alias for the @material-ui/styled-engine to point to @material-ui/styled-engine-cs. (see example usage in the babel.config.js for the docs)

Additional comments:

While reviewing, please consider reviewing the styled-component implementation as well, by commenting line 24 in the docs/babel.config.js and uncommenting line 25.

Default emotion config:

  // Swap the comments on the next two lines for using the styled-components as style engine
  '@material-ui/styled-engine': '../packages/material-ui-styled-engine/src',
  // '@material-ui/styled-engine': '../packages/material-ui-styled-engine-sc/src',

styled-components config:

  // Swap the comments on the next two lines for using the styled-components as style engine
  // '@material-ui/styled-engine': '../packages/material-ui-styled-engine/src',
  '@material-ui/styled-engine': '../packages/material-ui-styled-engine-sc/src',

Next steps

@@ -20,30 +21,56 @@ export default function ContinuousSlider() {
setValue(newValue);
};

const theme = {
Copy link
Member Author

Choose a reason for hiding this comment

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

These changes are added just for testing, will be reverted before merging

Copy link
Member

Choose a reason for hiding this comment

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

A reminder to remove the change from the demo. Maybe we could transform that into tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

Now that the components are moved to the lab these are separate examples, but it makes sense to move these in the tests now :))

Copy link
Member Author

Choose a reason for hiding this comment

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

Added tests, not sure if it makes sense to keep that per component, but at least we have it for now to make sure we don't regress during development

const Slider = React.forwardRef(function Slider(inputProps, inputRef) {
const props = useThemeProps(inputProps, inputRef, 'MuiSlider');
const classes = useThemeClasses('MuiSlider');
const themeVariantsClasses = useThemeVariants({ ...props, classes }, 'MuiSlider');
Copy link
Member Author

Choose a reason for hiding this comment

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

Slider is not really supporting variants, but it is added here for POC that it will work

@mbrookes
Copy link
Member

mbrookes commented Sep 1, 2020

Just a couple of thoughts:

  1. What if the names were Slider & SliderUnstyled (rather than SliderStyled & Slider), so that the current import brings in the expected component.

  2. What if instead of unstyled, the base component is "minimally styled" - the foundations on which Material Design or any other design system could be layered?

@mnajdova
Copy link
Member Author

mnajdova commented Sep 2, 2020

What if the names were Slider & SliderUnstyled (rather than SliderStyled & Slider), so that the current import brings in the expected component.

Totally, will rename them. I am still thinking of potential names for the SliderUnstyled, some options that we may consider apart for SliderUnstyled are: SliderBase, SliderHeadless, SliderVanilla, SliderNaked.

What if instead of unstyled, the base component is "minimally styled" - the foundations on which Material Design or any other design system could be layered?

I'd like ideally the unstyled or base version of component to know nothing about our styling mechanism, so people can opt out and style it from scratch, or even choose a different styling engine, maybe plain css, or something else. On the minimally styled version, I think maybe a better approach to be to export a theme that would provide that, rather than different version of the component. This way we may decide to add "Switch theme" capability for showing different themes on our docs page. These are just thoughts I have at this moment, I may change my mind, so will definitely keep this in mind

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 2, 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.

A first quick review

docs/src/pages/components/slider/CustomizedSlider.js Outdated Show resolved Hide resolved
packages/material-ui/src/Slider/index.js Outdated Show resolved Hide resolved
packages/material-ui/src/Slider/Slider.js Outdated Show resolved Hide resolved
packages/material-ui/src/Slider/Slider.js Outdated Show resolved Hide resolved
packages/material-ui/src/Slider/Slider.js Outdated Show resolved Hide resolved
packages/material-ui/src/Slider/Slider.js Outdated Show resolved Hide resolved
packages/material-ui/src/Slider/SliderBase.js Outdated Show resolved Hide resolved
packages/material-ui/src/Slider/SliderBase.js Outdated Show resolved Hide resolved
packages/material-ui/src/Slider/SliderBase.js Outdated Show resolved Hide resolved
@mbrookes
Copy link
Member

mbrookes commented Sep 2, 2020

I think maybe a better approach to be to export a theme that would provide that

The idea would be to provide a baseline style on which a theme would build, not a complete theme in its own right. (See Reach UI & Reakit). For example: SliderBase - unstyled; Slider - baseline styling; default theme - Material-UI.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 2, 2020

The idea would be to provide a baseline style on which a theme would build, not a complete theme in its own right. (See Reach UI & Reakit). For example: SliderBase - unstyled; Slider - baseline styling; default theme - Material-UI.

@mbrookes Would developers use it? For instance, if we look at the open source projects using Reakit and Reach UI on GitHub, e.g WordPress or welcome-ui what can we conclude? Do they use the baseline styles too? Without looking, my intuition would be that they don't and that it goes against the motivation for using the unstyled components in the first place. I would be happy to be proven wrong. We could also ask @gregberge as as happy user of Reakit: How do you best enjoy styling Reakit? Starting from a blank state of from their default styles?

I think that we can solve this baseline style with a second theme, inspired by Tailwind, Bootstrap, and Chakra UI: no shadows, system font, outlined two pixels, etc. Something simple.

@mbrookes
Copy link
Member

mbrookes commented Sep 2, 2020

Here's an unstyled slider:

image

(I left the label in so that you can see where the slider is.) Where do you start with customising that?!

But it isn't for me to speak for others – and asking one user what they prefer is the worst kind of anecdata! (No offence to @gregberge).

Instead, I'm proposing an approach which provides unstyled, a baseline, and a fully baked theme as the starting point for customisation - a "win win win" situation.

  • If you want to start with nothing, you wrap the *Base components (we have a naming conflict with ButtonBase BTW), probably copying and editing the provided wrapper components as a starting point.

  • If you want start with something that functions in the browser, you start with the baseline style components.

  • If you want to start with something fully baked, you use the Material (or other) theme.
    ( My main concern would be whether theme based styles could be efficiently pruned without a build-system specific plugin.)

Tailwind, Bootstrap, and Chakra UI

Those are two complete (if minimal) design systems. (Two because Chakra is basically Tailwind, so why mention it?). Instead, I'm suggesting the possibility of components that work "out of the box", but are otherwise design-system agnostic (if unthemed).

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 2, 2020

Here's an unstyled slider:

@mbrookes True. I imagine the problem would be identical to many components. If you take the data grid, good luck if you start from a blank state like the slider. We could ask developers on #6218. This is an interesting source to benchmark: JedWatson/react-select#2706.

I wonder if a baseline theme, something that is easier to customize really exists for the slider or the data grid. Where would the simplification happen and make a non-negligible difference?

Maybe it's about the inversion of control, maybe if we provide a simple demo with the styles exposed, using the unstyled slider, we would hit the target 🎯.

@mbrookes
Copy link
Member

mbrookes commented Sep 2, 2020

maybe if we provide a simple demo with the styles exposed, using the unstyled slider, we would hit the target

That could be a good middle ground. 👍

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 21, 2020

I have turned the above exploration into #22435. I don't understand how the server-side works with third-party packages without the componentId of styled-components. It seems to be just fine but no idea how.


I have looked at emotion integration with Next.js. I don't think that the current setup is OK:

Capture d’écran 2020-09-21 à 23 20 37

https://validator.w3.org/nu/?doc=https%3A%2F%2Fdeploy-preview-22435--material-ui.netlify.app%2Fcomponents%2Fslider-styled%2F

I have tested, we can solve the issue with the "official" integration example. It uses a custom cache to step emotion to inject CSS inside the body and an extractor function to inline it in the head.

@Andarist are the imports going to change in emotion v11? Are you aware of this line?

@Andarist
Copy link
Contributor

I have looked at emotion integration with Next.js. I don't think that the current setup is OK:

We are aware about this validator reports but this doesn't have any downsides in practice. This strategy is allowing the so-called 0 config SSR because we can just render (and stream the result!) without any extra post processing steps. If you want style elements to be put in <head/> this requires rendering the whole thing first and then parsing the outcome and moving things around so the adjusted output can be shipped to the client. We provide a package for all of this but it becomes way less 0 config if you go that route.

@Andarist are the imports going to change in emotion v11?

Yes, but only a little bit and we provide a codemod for this. Changes are - different pkg name (@emotion/core -> @emotion/react) and what has been previously in emotion-theming package is now exported from @emotion/react. We are close to shipping v11 so I assume this won't affect you much because you will be able to "migrate" before your stuff hits the stable mark, right?

Are you aware of this line?

I assume that this is pretty much equivalent of:

import createCache from '@emotion/cache'
const cache = createCache({ key: 'mui' })
cache.compat = true

The only utilize the builtin cache from emotion that has this flag already set. It's a flag suppressing some warnings that are in place to validate if the rendered styles are compatible with 0 config SSR. If one is not interested in that they can set this flag and we assume that the non-0 config SSR approach will be used by the consumer.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 22, 2020

We are aware about this validator reports but this doesn't have any downsides in practice.

I have found whatwg/html#1605 on this matter. It might not have downside effects in practice, however, I have a concern with the noise it creates in the output, making it harder for us to identify other fails in https://validator.w3.org/.

@mnajdova
Copy link
Member Author

@oliviertassinari improved the next.js + emotion integration

image

was fixed with those changes 👍

@Andarist
Copy link
Contributor

This PR has a lot of comments and I can't find the one that I would like to comment so... just going to do it here.

I've consulted Mitchell on why react-select specifies @emotion/core as a dep rather than a peerDep:

  1. it's mainly to create less friction - but if pkg managers start to auto-install peer deps then this might change in the future
  2. it's somewhat safe in their case because they don't expose theme anywhere, so at least mismatch on this singleton can't cause any issues
  3. caches can mismatch, but the worst case scenario some styles can get injected twice to the document - so they have decided that it's not a big deal
  4. this can be a problem if one uses a custom CacheProvider but they consider such people to be advanced users and that they should take care of ensuring that there is only a single @emotion/core in the dep tree, using yarn deduplicate, resolutions, or whatever

@eps1lon
Copy link
Member

eps1lon commented Sep 22, 2020

I've consulted Mitchell on why react-select specifies @emotion/core as a dep rather than a peerDep:

It can also lead to bundling @emotion/core twice if app devs don't pay attention to their lockfile. I have to check with berry devs how their status is on automatic peer deps but we can't really rely on it as long as yarn v1 exists. Though once npm starts doing it my prototyping concern is largely satisfied since prototyping usually uses the latest tooling anyway.

@mnajdova mnajdova dismissed stale reviews from eps1lon and mbrookes September 23, 2020 12:26

Requested changes implemented

@mnajdova mnajdova merged commit 56fae30 into mui:next Sep 23, 2020
@zannager zannager added the component: slider This is the name of the generic UI component, not the React module! label Feb 3, 2023
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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants