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] - style rule names don't match other component's API and don't accept styles as expected #18010

Closed
2 tasks done
hoop71 opened this issue Oct 23, 2019 · 8 comments · Fixed by #18011
Closed
2 tasks done
Labels
component: slider This is the name of the generic UI component, not the React module! new feature New feature or request

Comments

@hoop71
Copy link
Contributor

hoop71 commented Oct 23, 2019

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

When trying to access the disabled rule of the thumb rule the styles aren't applied as expected
example 1 - Doesn't apply styles

thumb: {
    '$disabled': {
      // styles
    }
  },

instead, you must apply them like with an unexpected &
example 2 - Applies styles

thumb: {
    '$disabled &': {
      // styles
    }
  },

Expected Behavior 🤔

You should be able to apply classes in the same was as other MUI components.

thumb: {
    '$disabled': {
      // styles
    }
  },

Steps to Reproduce 🕹

https://codesandbox.io/s/create-react-app-kkfhk

There are toggles to show the proposed solution. Slider must be disabled to see current issue and patch.

Steps:

  1. import Slider and use makeStyles hook to add custom styles
  2. pass classes to the as shown above

Context 🔦

  • I want the Slider styles to match the rest of the MUI API
  • I want to be able to style the disabled thumb

Your Environment 🌎

Tech Version
Material-UI v4.5.1
React 16.9.0
Browser Version 77.0.3865.120 (Official Build) (64-bit)
TypeScript No
etc.
@eps1lon
Copy link
Member

eps1lon commented Oct 24, 2019

Yeah I can see how this is unfortunate. @oliviertassinari Can we make this work without special syntax?

@eps1lon eps1lon added component: slider This is the name of the generic UI component, not the React module! discussion labels Oct 24, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 24, 2019

@hoop71 The current CSS selector is:

.Mui-disabled .MuiSlider-thumb {
  /* foo */
}

We apply the disabled pseudo-class once on the root of the slider. I would probably do:

import styled from "styled-components";

const StyledSlider = styled(Slider)`
  &.Mui-disabled {
    .MuiSlider-thumb {
      background-color: yellow;
    }
  }
`;

https://codesandbox.io/s/create-react-app-rpv13

But maybe we should output the following to avoid leaks.

.MuiSlider-root.Mui-disabled .MuiSlider-thumb {
  /* foo */
}

From what I understand, you would like to do:

.Mui-disabled.MuiSlider-thumb {
  /* foo */
}

@oliviertassinari
Copy link
Member

Ok, I have updated #18011 with an alternative proposal for the disabled state. It's closer to what we do in the rest of the codebase. I have left the vertical state unchanged.

@oliviertassinari oliviertassinari added new feature New feature or request and removed discussion labels Oct 24, 2019
@hoop71
Copy link
Contributor Author

hoop71 commented Oct 24, 2019

@hoop71 The current CSS selector is:

.Mui-disabled .MuiSlider-thumb {
  /* foo */
}

We apply the disabled pseudo-class once on the root of the slider. I would probably do:

import styled from "styled-components";

const StyledSlider = styled(Slider)`
  &.Mui-disabled {
    .MuiSlider-thumb {
      background-color: yellow;
    }
  }
`;

https://codesandbox.io/s/create-react-app-rpv13

But maybe we should output the following to avoid leaks.

.MuiSlider-root.Mui-disabled .MuiSlider-thumb {
  /* foo */
}

From what I understand, you would like to do:

.Mui-disabled.MuiSlider-thumb {
  /* foo */
}

@oliviertassinari @eps1lon Yes this all makes sense. I do think it makes sense to be able to style like below without having to get the root involved.

.Mui-disabled.MuiSlider-thumb {
  /* foo */
}

I guess I am also unsure of what the & at the end of the rule is there for. In the below case, what is '$vertical &' accomplishing?

  /* Styles applied to the rail element. */
  rail: {
    display: 'block',
    position: 'absolute',
    width: '100%',
    height: 2,
    borderRadius: 1,
    backgroundColor: 'currentColor',
    opacity: 0.38,
    '$vertical &': {
      height: '100%',
      width: 2,
    },
  },

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 24, 2019

You need to think in terms of CSS selectors.

$ reference another style rule
& reference the parent style rule
Each style rule has a class name

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 24, 2019

what is '' accomplishing?

You can resolve this selector manually:

  • $vertical &
  • $vertical $rail
  • .MuiSlider-vertical .MuiSlider-rail

@oliviertassinari
Copy link
Member

Did you have a look at the changes I propose, does it match your problem?

@hoop71
Copy link
Contributor Author

hoop71 commented Oct 24, 2019

Did you have a look at the changes I propose, does it match your problem?

Yes! Looks good and should solve our needs. Thanks for your fast work as always!

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 a pull request may close this issue.

3 participants