[Slider][Badge] Fix classes prop not working #24034

mnajdova · 2020-12-17T10:39:50Z

mui-pr-bot · 2020-12-17T10:42:29Z

@material-ui/core: parsed: +0.08% , gzip: +0.05%

Generated by 🚫 dangerJS against 78c86e8

mnajdova · 2020-12-17T10:44:07Z

One issue I see is with the pseudo selectors: disabled, active etc. We cannot currently override those via classes.disabled or classes.active

Another issue I found is, I am considering the vertical as a pseudo selector as well, I believe we should change the logic and check for the prop there, instead of applying the overrides under subselector, as classes.vertical will not work...

So generally, is it good rule of thumb, to always use the props and apply some styles conditionally, unless the props are related to some global selector, like disabled, active etc.. cc @oliviertassinari

Edit:

I meant should we refactor this kinds of styles - https://github.com/mui-org/material-ui/pull/24034/files#diff-4bc6eb4ab21a45337e0ab1a81a8bda390122ca0d7b040b3f2a3d7ca0f24232d8R229-R232

-'&.MuiSlider-vertical' {}
+...(props.styleProps.vertical && {}),

mnajdova · 2020-12-17T10:44:42Z

We should also probably add more tests, that the classes prop will be working as expected (currently we are testing only the utility classes as far as I could see

oliviertassinari · 2020-12-17T10:49:11Z

One issue I see is with the pseudo selectors: disabled, active etc. We cannot currently override those via classes.disabled or classes.active

@mnajdova What do you mean by override? Do you mean "we can't provide a custom class name" which would be a bug or "we can't override the CSS" which sounds expected.

I am considering the vertical as a pseudo selector as well,

It could have been better in the past to use .Mui-active for the active labels, it's what would have been more consistent. For the vertical slider, I think that we have historically increased the specificity for simplicity when writing the CSS, to avoid an exposition of style rules. But in any case, I think that as long as v5 doesn't increase the specificity, it's not breaking.

mnajdova · 2020-12-17T11:00:44Z

@mnajdova What do you mean by override? Do you mean "we can't provide a custom class name" which would be a bug or "we can't override the CSS" which sounds expected.

Will fix the component to accept custom classes.disabled classname, for the overrides my assumption was wrong, of course the specificity is bigger and it cannot be overridden if it is not again with the same specificity.

Let me then refactor the vertical slider with these changes and we can test that as well in the end

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

…d.js

mnajdova · 2020-12-17T11:35:05Z

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

@@ -747,9 +757,8 @@ const SliderUnstyled = React.forwardRef(function SliderUnstyled(props, ref) {
              onMouseLeave={handleMouseLeave}
              {...thumbProps}
              className={clsx(utilityClasses.thumb, thumbProps.className, {
-                [sliderUnstyledClasses['active']]: active === index,
-                [sliderUnstyledClasses['disabled']]: disabled,


It is already applied in the useSliderClasses

mnajdova · 2020-12-17T11:39:55Z

@oliviertassinari should we also merge this one mnajdova#20 for removing the usage of invisible classes selectors in the Badge component?

oliviertassinari · 2020-12-17T11:46:01Z

should we also merge this one mnajdova#20 for removing the usage of invisible classes selectors in the Badge component?

No preferences

eps1lon · 2020-12-17T11:47:37Z

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

@@ -40,7 +40,6 @@ const BadgeUnstyled = React.forwardRef(function BadgeUnstyled(props, ref) {
      vertical: 'top',
      horizontal: 'right',
    },
-    // eslint-disable-next-line @typescript-eslint/no-unused-vars


Do we need a lint rule to disable eslint-disable for some rules? 😁

Hahahahha, nah it's just me :D

mnajdova · 2020-12-17T11:48:03Z

should we also merge this one mnajdova#20 for removing the usage of invisible classes selectors in the Badge component?

No preferences

I'll merge it to have one PR complete all the necessary changes I found at this moment. If we receive another problem, we can revisit later again

EDIT: WON'T MERGE

Reverted the changes, as this is how it was implemented in v4.

badge invisible

…rovements Revert "badge invisible"

…najdova/material-ui into feat/slider-classes-prop-fixes

wip

b31749a

oliviertassinari added the bug 🐛 Something doesn't work label Dec 17, 2020

github-actions bot mentioned this pull request Dec 17, 2020

Update snapshots for pr/24034 eps1lon/mui-scripts-incubator#1535

Closed

mnajdova added 2 commits December 17, 2020 12:16

fixed pseudo classes, fixed vertical styles

8b779dd

lint fixes

92f2ce2

mnajdova requested a review from oliviertassinari December 17, 2020 11:24

mnajdova commented Dec 17, 2020

View reviewed changes

packages/material-ui-unstyled/src/SliderUnstyled/SliderUnstyled.js Outdated Show resolved Hide resolved

Update packages/material-ui-unstyled/src/SliderUnstyled/SliderUnstyle…

8d23e70

…d.js

mnajdova commented Dec 17, 2020

View reviewed changes

badge invisible

58d12b7

oliviertassinari approved these changes Dec 17, 2020

View reviewed changes

eps1lon reviewed Dec 17, 2020

View reviewed changes

mnajdova added 6 commits December 17, 2020 12:48

Merge pull request #20 from mnajdova/feat/classes-badge-improvements

de76d7b

badge invisible

final fixes on where the classes are applied

cef8fe9

Revert "badge invisible"

f896942

Merge pull request #21 from mnajdova/revert-20-feat/classes-badge-imp…

6467815

…rovements Revert "badge invisible"

reverted last track changes

5e08494

Merge branch 'feat/slider-classes-prop-fixes' of https://github.com/m…

78c86e8

…najdova/material-ui into feat/slider-classes-prop-fixes

oliviertassinari added component: badge This is the name of the generic UI component, not the React module! component: slider This is the name of the generic UI component, not the React module! labels Dec 17, 2020

oliviertassinari changed the title ~~[Slider, Badge] Fix classes prop not working~~ [Slider][Badge] Fix classes prop not working Dec 17, 2020

mnajdova merged commit 5d31a60 into mui:next Dec 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Slider][Badge] Fix classes prop not working #24034

[Slider][Badge] Fix classes prop not working #24034

mnajdova commented Dec 17, 2020 •

edited

Loading

mui-pr-bot commented Dec 17, 2020 •

edited

Loading

mnajdova commented Dec 17, 2020 •

edited

Loading

mnajdova commented Dec 17, 2020

oliviertassinari commented Dec 17, 2020 •

edited

Loading

mnajdova commented Dec 17, 2020

mnajdova Dec 17, 2020 •

edited

Loading

mnajdova commented Dec 17, 2020

oliviertassinari commented Dec 17, 2020

eps1lon Dec 17, 2020

mnajdova Dec 17, 2020

mnajdova commented Dec 17, 2020 •

edited

Loading

[Slider][Badge] Fix classes prop not working #24034

[Slider][Badge] Fix classes prop not working #24034

Conversation

mnajdova commented Dec 17, 2020 • edited Loading

mui-pr-bot commented Dec 17, 2020 • edited Loading

mnajdova commented Dec 17, 2020 • edited Loading

mnajdova commented Dec 17, 2020

oliviertassinari commented Dec 17, 2020 • edited Loading

mnajdova commented Dec 17, 2020

mnajdova Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

mnajdova commented Dec 17, 2020

oliviertassinari commented Dec 17, 2020

eps1lon Dec 17, 2020

Choose a reason for hiding this comment

mnajdova Dec 17, 2020

Choose a reason for hiding this comment

mnajdova commented Dec 17, 2020 • edited Loading

EDIT: WON'T MERGE

mnajdova commented Dec 17, 2020 •

edited

Loading

mui-pr-bot commented Dec 17, 2020 •

edited

Loading

mnajdova commented Dec 17, 2020 •

edited

Loading

oliviertassinari commented Dec 17, 2020 •

edited

Loading

mnajdova Dec 17, 2020 •

edited

Loading

mnajdova commented Dec 17, 2020 •

edited

Loading