Skip to content

[Slider] Adjust css to match the specification#26632

Merged
siriwatknp merged 23 commits intomui:nextfrom
siriwatknp:fix/slider-design
Jun 17, 2021
Merged

[Slider] Adjust css to match the specification#26632
siriwatknp merged 23 commits intomui:nextfrom
siriwatknp:fix/slider-design

Conversation

@siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jun 7, 2021 •
edited
Loading

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 scope: slider Changes related to the slider. label Jun 7, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Jun 7, 2021 •
edited
Loading

Details of bundle changes (experimental)

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

Generated by 🚫 dangerJS against 33f34fa

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 7, 2021

@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 •
edited
Loading

@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

oliviertassinari
oliviertassinari reviewed Jun 7, 2021
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment •
edited
Loading

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)

@siriwatknp siriwatknp marked this pull request as ready for review June 8, 2021 07:15
@siriwatknp

This comment has been minimized.

Sign in to view
@siriwatknp siriwatknp added the breaking change Introduces changes that are not backward compatible. label Jun 8, 2021
mnajdova
mnajdova approved these changes Jun 11, 2021
View reviewed changes
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

siriwatknp commented Jun 11, 2021

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 •
edited
Loading

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.

oliviertassinari
oliviertassinari reviewed Jun 13, 2021
View reviewed changes
@siriwatknp
Copy link
Member Author

siriwatknp commented Jun 14, 2021

Things I could notice: vertical alignment issue on this demo

Fixed.

@siriwatknp
Copy link
Member Author

siriwatknp commented Jun 14, 2021 •
edited
Loading

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 •
edited
Loading

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
oliviertassinari
oliviertassinari reviewed Jun 15, 2021
View reviewed changes
oliviertassinari
oliviertassinari approved these changes Jun 16, 2021
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment •
edited
Loading

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

oliviertassinari commented Jun 16, 2021

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
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oliviertassinari oliviertassinari oliviertassinari approved these changes

@mnajdova mnajdova mnajdova approved these changes

Assignees

No one assigned

Labels

breaking change Introduces changes that are not backward compatible. design: material This is about Material Design, please involve a visual or UX designer in the process scope: slider Changes related to the slider.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Slider] Update to match the specification

4 participants

@siriwatknp @mui-pr-bot @oliviertassinari @mnajdova