[Slider] Replace core Slider with SliderStyled #23308

mnajdova · 2020-10-29T13:20:25Z

Breaking change

[Slider] Move the implementation from @material-ui/styles to @material-ui/styled-engine.

This PR replaced the Slider in @material-ui/core with the SliderStyled component from the lab.

Changes done as part of the PR:
The files from @material-ui/lab/SliderStyled files are now replacing the @material-ui/core/Slider. During the replacement we found several bugs that were not caught previosly because we didn't have screenshot comparison with the Base slider. All of them are fixed now.

In addition to this, we renamed the ValueLabelUnstyled components to SliderValueLabelUnstyled. The component ValueLabelStyled was removed and the styles are now part of the SliderStyled in order to ensure we are consistent with the specificity of two for all slots inside the Slider.

mui-pr-bot · 2020-10-29T13:22:45Z

@material-ui/core: parsed: +1.29% , gzip: +1.47%
@material-ui/lab: parsed: -10.76% 😍, gzip: -13.65% 😍

Details of bundle changes

Generated by 🚫 dangerJS against 641be42

mnajdova · 2020-11-17T15:02:47Z

@oliviertassinari & @eps1lon I would kindly ask you for final review, as I've addressed the last comment #23308 (comment)

mnajdova · 2020-11-17T15:44:33Z

docs/pages/api-docs/slider.md

 | <span class="prop-name">defaultValue</span> | <span class="prop-type">Array&lt;number&gt;<br>&#124;&nbsp;number</span> |  | The default element value. Use when the component is not controlled. |
-| <span class="prop-name">disabled</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, the slider is disabled. |
+| <span class="prop-name">disabled</span> | <span class="prop-type">bool</span> |  | If `true`, the slider is disabled. |


The defaults are now in the SliderUnstyled, not here

oliviertassinari

I have notices some aspects that require follow-ups, but that doesn't necessarily have to prevent merging this pull request.

oliviertassinari · 2020-11-18T09:18:09Z

docs/pages/api-docs/slider.md

-| <span class="prop-name">min</span> | <span class="prop-type">number</span> | <span class="prop-default">0</span> | The minimum allowed value of the slider. Should not be equal to max. |
+| <span class="prop-name">isRtl</span> | <span class="prop-type">bool</span> |  | Indicates whether the theme context has rtl direction. It is set automatically. |
+| <span class="prop-name">marks</span> | <span class="prop-type">Array&lt;{ label?: node, value: number }&gt;<br>&#124;&nbsp;bool</span> |  | Marks indicate predetermined values to which the user can move the slider. If `true` the marks are spaced according the value of the `step` prop. If an array, it should contain objects with `value` and an optional `label` keys. |
+| <span class="prop-name">max</span> | <span class="prop-type">number</span> |  | The maximum allowed value of the slider. Should not be equal to min. |


Default values are missing. What do you think of "pulling" the values from SliderUnstyled?

Let me do that in a follow up PR as it will require again changes in the buildAPI, it would be easier to have it as a separate change. Will leave this open for visibility

oliviertassinari · 2020-11-18T09:23:22Z

docs/src/pages/components/slider/CustomizedSlider.js

    left: 'calc(-50% + 12px)',
    top: -22,
    '& *': {
      background: 'transparent',
      color: '#000',
    },
  },
-  track: {
+  '& .MuiSlider-track': {


It's going from "what name does this identifier have?" to

@eps1lon It will likely be simpler for most developers in v5. We have been regularly linking https://material-ui.com/styles/advanced/#with-material-ui-core in v4 when developers were struggling to understand how this classes API work. It's not obvious for many. I think that the new syntax will work better because this it's closer to what developers are already familiar with: CSS/SASS. Another positive signal is the volume of complaints around hard to customize components seems to have gone down between v3 and v4 once we introduced the global class names.

Ideally it should've been part of the RFC process.

This is one of the first dimensions we have considered. I believe the section "Deterministic classnames on the components that can be targeted for custom styles" surfaces this, but we could have exposed it deeper.

oliviertassinari · 2020-11-18T09:33:45Z

packages/material-ui-unstyled/src/SliderUnstyled/SliderUnstyled.js

@@ -11,6 +11,7 @@ import {
  unstable_capitalize as capitalize,
  unstable_useControlled as useControlled,
 } from '@material-ui/utils';
+import sliderClasses from './sliderClasses';


Do we need a different file to host this file? If we do, should we write it in TypeScript directly?

I've extracted it in a separate file as it needs to be used in both components files (Slider & SliderValueLabel). I think it's better to have it separately for easier navigation. Converting it to ts makes sense, let me do that 👍

There are some ts errors when converting this to ts file - https://app.circleci.com/pipelines/github/mui-org/material-ui/27322/workflows/7aed2605-b9fb-42e8-81d1-5b6e10ccb916/jobs/197097. Let me leave it as is and address as a follow up PR as it seems like we need to do changes with the build

oliviertassinari · 2020-11-18T09:34:23Z

packages/material-ui-unstyled/src/SliderUnstyled/SliderUnstyled.js

      classes['root'],
-      getUtilityClass(`color${capitalize(color)}`),
+      sliderClasses[`color${capitalize(color)}`],


Will we get requests to support <Slider color="error" />? No idea.

I see the concern, should we maybe use the dynamic method for dynamic props? And have in the sliderClasses the ones we support by default?

On the other hand we could not do this changes, and leave as it was before with the getUtilityClass utility, and export hte classes just for the sake of the clients being able to use them, and use them also in the tests...

I don't know. The propTypes are another bottlneck. I think that we can wait.

Alright, let's leave it open again for visibility

eps1lon · 2020-11-18T10:29:12Z

It's not obvious for many. I think that the new syntax will work better because this it's closer to what developers are already familiar with: CSS/SASS

How did you get that impression?
How is that an argument? Should we use bit-wise operations because people are already familiar with JS?

Another positive signal is the volume of complaints around hard to customize components seems to have gone down between v3 and v4 once we introduced the global class names.

This doesn't make it easier, objectively.

oliviertassinari · 2020-11-19T11:31:44Z

🥁🥁🥁🥁🥁🥁🥁🥁🥁🥁🥁

oliviertassinari · 2020-11-19T11:32:53Z

I can't wait to see how developers upgrade and play with the new slider :D

mnajdova added 16 commits September 25, 2020 17:15

add emotion peer dependencies

c2d31cc

fixed types & tests

5ae933f

prettier

18b0668

Merge branch 'next' of https://github.com/mui-org/material-ui into next

f0ef95c

Merge branch 'next' of https://github.com/mui-org/material-ui into next

c7bebb8

Merge branch 'next' of https://github.com/mui-org/material-ui into next

92b2d6e

Merge branch 'next' of https://github.com/mui-org/material-ui into next

13da531

Merge branch 'next' of https://github.com/mui-org/material-ui into next

cf5d9a5

Merge branch 'next' of https://github.com/mui-org/material-ui into next

b8d1291

Merge branch 'next' of https://github.com/mui-org/material-ui into next

a9d8690

Merge branch 'next' of https://github.com/mui-org/material-ui into next

497830a

extract components

cedbe3b

Merge branch 'next' of https://github.com/mui-org/material-ui into next

d50ea1e

Merge branch 'next' of https://github.com/mui-org/material-ui into next

0f6a4cd

Merge branch 'next' of https://github.com/mui-org/material-ui into next

c8bde3f

init

f3ba419

mnajdova added 7 commits October 29, 2020 14:26

imports fixes

d9b1475

imports order

18ce348

testing umd build

fc94e1d

prettier

8573dfe

changed imports

22f4581

fixes

f1eb139

fixes

49f0799

github-actions bot mentioned this pull request Oct 29, 2020

Update snapshots for pr/23308 eps1lon/mui-scripts-incubator#1231

Closed

mnajdova added 5 commits October 29, 2020 16:29

wip

c11de66

proptypes fixed

25a0da4

framer

65b91c5

build fix test

b1b38bf

framer & build test

cf9f1c8

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 17, 2020

Merge branch 'next' into feat/move-slider-styled-to-core

081852a

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 17, 2020

docs:api

28336f1

mnajdova commented Nov 17, 2020

View reviewed changes

mnajdova requested review from oliviertassinari and eps1lon November 17, 2020 15:50

trigger build

6bad28d

oliviertassinari approved these changes Nov 18, 2020

View reviewed changes

mnajdova added 2 commits November 18, 2020 11:00

converted sliderClasses to ts

663df7b

broke circular dep

654419e

mnajdova added 3 commits November 18, 2020 11:52

reverted slider classes changes

ac5e3c3

prettier

c042f82

Merge branch 'next' of https://github.com/mui-org/material-ui into next

f7af03f

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 18, 2020

Merge branch 'next' into feat/move-slider-styled-to-core

d67bb6f

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 18, 2020

prettier

19f7742

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 18, 2020

Merge branch 'next' into feat/move-slider-styled-to-core

641be42

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 19, 2020

mnajdova merged commit 4b71794 into mui:next Nov 19, 2020

oliviertassinari mentioned this pull request Nov 19, 2020

[RFC] Material-UI v5 🚀 #20012

Closed

mnajdova mentioned this pull request Nov 19, 2020

[docs] Generate default values for docs from the unstyled components #23614

Merged

oliviertassinari mentioned this pull request Jan 12, 2021

Optimize bundle size (now 317KB with v4) #24381

Closed

eps1lon mentioned this pull request Feb 15, 2021

[Portal] Migrate to unstyled #24890

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Slider] Replace core Slider with SliderStyled #23308

[Slider] Replace core Slider with SliderStyled #23308

mnajdova commented Oct 29, 2020 •

edited by oliviertassinari

Loading

mui-pr-bot commented Oct 29, 2020 •

edited

Loading

mnajdova commented Nov 17, 2020 •

edited

Loading

mnajdova Nov 17, 2020

oliviertassinari left a comment

oliviertassinari Nov 18, 2020

mnajdova Nov 18, 2020

oliviertassinari Nov 18, 2020

oliviertassinari Nov 18, 2020

mnajdova Nov 18, 2020

mnajdova Nov 18, 2020

mnajdova Nov 18, 2020

oliviertassinari Nov 18, 2020

mnajdova Nov 18, 2020

mnajdova Nov 18, 2020

oliviertassinari Nov 18, 2020

mnajdova Nov 18, 2020

eps1lon commented Nov 18, 2020

oliviertassinari commented Nov 19, 2020

oliviertassinari commented Nov 19, 2020

[Slider] Replace core Slider with SliderStyled #23308

[Slider] Replace core Slider with SliderStyled #23308

Conversation

mnajdova commented Oct 29, 2020 • edited by oliviertassinari Loading

Breaking change

mui-pr-bot commented Oct 29, 2020 • edited Loading

mnajdova commented Nov 17, 2020 • edited Loading

Choose a reason for hiding this comment

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eps1lon commented Nov 18, 2020

oliviertassinari commented Nov 19, 2020

oliviertassinari commented Nov 19, 2020

mnajdova commented Oct 29, 2020 •

edited by oliviertassinari

Loading

mui-pr-bot commented Oct 29, 2020 •

edited

Loading

mnajdova commented Nov 17, 2020 •

edited

Loading