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] Adjust css to match the specification #26632

Merged
merged 23 commits into from
Jun 17, 2021

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jun 7, 2021

BREAKING CHANGE


Closes #20153
Preview: https://deploy-preview-26632--material-ui.netlify.app/components/slider/

  • adjustment based on material design spec

  • add size="small" and test

    image

  • add MusicPlayer demo light & dark mode (to test customisation)

    image

I use this opportunity to refactor the CSS approach due to these issues I experienced.

  • height defined on all of the elements (root, rail, track) which means I need to change all of the places.
  • change height(thickness) affect the position of thumb, as a result need to adjust thumb margin and track, rail radius.
  • change height(thickness) affect mark to not aligned center.

Goal

to have these experiences when customizing the slider.

  • change height(thickness) only at root and rail, track should scale
  • border-radius of rail, track inherit from root, so developers only need to adjust border-radius at one place.
  • change height(thickness) then thumb should still be centered
  • change thumb's size(width, height) should make it still centered
  • change label size should make it float on the thumb perfectly
  • box-shadow of thumb use :before so that it is easier to remove and not conflict with existing box-shadow

Result

Default Slider

Capture d’écran 2021-06-14 à 13 22 42

Customized Slider

const StyledSlider = styled(Slider)({
  color: 'red',
  height: 2,
});

Screen Shot 2564-06-15 at 17 28 18

const StyledSlider = styled(Slider)({
  color: 'orange',
  height: 8,
  '& .MuiSlider-thumb': {
    width: 16,
    height: 16,
  },
});

Screen Shot 2564-06-15 at 17 29 45

@siriwatknp siriwatknp added the component: slider This is the name of the generic UI component, not the React module! label Jun 7, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Jun 7, 2021

Details of bundle changes (experimental)

@material-ui/core: parsed: +0.29% , gzip: +0.17%

Generated by 🚫 dangerJS against 33f34fa

@oliviertassinari
Copy link
Member

@siriwatknp Do you have thoughts about this comment #22782 (comment), on the "size"/"density" aspect of the change?

@oliviertassinari oliviertassinari added the design: material This is about Material Design, please involve a visual or UX designer in the process label Jun 7, 2021
@siriwatknp siriwatknp marked this pull request as draft June 7, 2021 10:27
@siriwatknp
Copy link
Member Author

siriwatknp commented Jun 7, 2021

@oliviertassinari with the new approach that I made, dense is not hard to add. If I understand you correctly, this is what you expect right?

<Slider dense />

the rail, track (thickness) will be smaller from 8px to 4px (may be) and thumb from 20px to 12px (may be). I would say we can do this with small afford and not impact customisation. The question is "do we want to put it on top of the spec or not"? I assume that developers won't have objection about dense since it is an extra thing but let's see other team's opinion @mui-org/core

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.

with the new approach that I made, dense is not hard to add. If I understand you correctly, this is what you expect right?

@siriwatknp I was wondering about something in this order. Something that would make it 1. easier for developers to migrate from v4 to v5, to the new design spec (especially when working on high-density dashboards) 2. allow to cover a wider range of use cases.

"do we want to put it on top of the spec or not"?

I think so. There are multiple precedents from us doing it.

<Slider dense />

For the API. If we envision use cases for different sizes through customizations, then an enum size prop would probably work better, otherwise dense.


Something seems broken with your screenshots in the PR's description, they are not at the right scale 😁 . I would expect:

Capture d’écran 2021-06-07 à 17 41 09

not:

Capture d’écran 2021-06-07 à 17 41 12

(on macOS GitHub automatically detect that my screenshot at 2:1 and resize them correctly, I don't know how it does that)

docs/src/pages/components/slider/CustomizedSlider.js Outdated Show resolved Hide resolved
@siriwatknp siriwatknp marked this pull request as ready for review June 8, 2021 07:15
@siriwatknp

This comment has been minimized.

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Changes look good to me. @oliviertassinari do we document changes like this as Breaking changes? We are for sure breaking any customization as the styles are different.

@siriwatknp
Copy link
Member Author

Changes look good to me. @oliviertassinari do we document changes like this as Breaking changes? We are for sure breaking any customization as the styles are different.

Added to migration-v4.md in Slider section describing css rework.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 13, 2021

How to do that? 😲

The only solution I know is to use macOS 😁. The docs don't help https://docs.github.com/en/github/writing-on-github/working-with-advanced-formatting/attaching-files.


Things I could notice: vertical alignment issue on this demo https://deploy-preview-26632--material-ui.netlify.app/components/slider/#continuous-sliders:

Capture d’écran 2021-06-13 à 16 44 31

It's even easier to spot when in touch pointer mode:

Capture d’écran 2021-06-13 à 16 45 02


Regarding the discussion on the density/size problem. @mnajdova Did you have any thoughts? I'm pinging @danilo-leal too.

@siriwatknp
Copy link
Member Author

Things I could notice: vertical alignment issue on this demo

Fixed.

@siriwatknp
Copy link
Member Author

siriwatknp commented Jun 14, 2021

Regarding the discussion on the density/size problem

2 ways I see,

  1. merge with current change, consult Danilo and work on beta.
  2. add size="small" in this PR to match v4 design, this could benefit people who want to keep the old design.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 14, 2021

I would add a third option: #26632 (comment)

  1. Add size="small" with the same design but a denser display, with the same or a bit less what the current size of the slider is in v4. Maybe it's actually the same as 2. 😁

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jun 15, 2021
packages/material-ui/src/Slider/Slider.test.js Outdated Show resolved Hide resolved
framer/Material-UI.framerfx/code/Slider.tsx Outdated Show resolved Hide resolved
packages/material-ui/src/Slider/Slider.d.ts 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
docs/src/pages/components/slider/MusicPlayerSlider.tsx Outdated Show resolved Hide resolved
docs/src/pages/components/slider/MusicPlayerSlider.tsx Outdated Show resolved Hide resolved
docs/src/pages/components/slider/slider.md Outdated Show resolved Hide resolved
docs/src/pages/components/slider/slider.md Outdated Show resolved Hide resolved
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.

Awesome 🔥

I have pushed two more polishes for the vertical slider

  1. it feels too crowded here:

Capture d’écran 2021-06-16 à 17 24 05

https://deploy-preview-26632--material-ui.netlify.app/components/slider/#vertical-sliders

  1. I have fixed the transition on the vertical slider

@oliviertassinari
Copy link
Member

I have rebased on HEAD to fix the prettier issue. We were a week behind. It's safer too.

@siriwatknp siriwatknp merged commit b8120ba into mui:next Jun 17, 2021
@siriwatknp siriwatknp deleted the fix/slider-design branch June 17, 2021 02:33
@siriwatknp siriwatknp mentioned this pull request Jun 23, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: slider This is the name of the generic UI component, not the React module! design: material This is about Material Design, please involve a visual or UX designer in the process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Slider] Update to match the specification
4 participants