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] Update style to match the specification #22782

Closed
wants to merge 7 commits into from

Conversation

joshwooding
Copy link
Member

@joshwooding joshwooding commented Sep 27, 2020

Closes #20153

Breaking Change

  • ValueLabel class circle changed to container

@joshwooding joshwooding added design: material This is about Material Design, please involve a visual or UX designer in the process component: slider This is the name of the generic UI component, not the React module! labels Sep 27, 2020
@@ -634,7 +634,7 @@ const SliderUnstyled = React.forwardRef(function SliderUnstyled(props, ref) {
<input value={values.join(',')} name={name} type="hidden" />
{marks.map((mark, index) => {
const percent = valueToPercent(mark.value, min, max);
const style = axisProps[axis].offset(percent);
const style = axisProps[axis].offset(clamp(percent, 0.5, 99));
Copy link
Member Author

Choose a reason for hiding this comment

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

prevents marks being displayed outside the track

@@ -198,14 +192,20 @@ const SliderRoot = muiStyled(
},
'& .MuiSlider-valueLabel': {
// IE 11 centering bug, to remove from the customization demos once no longer supported
Copy link
Member Author

Choose a reason for hiding this comment

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

We should still re-visit the centring styling once we drop IE 11

@mui-pr-bot
Copy link

mui-pr-bot commented Sep 27, 2020

@material-ui/lab: parsed: +0.22% , gzip: +0.18%

Details of bundle changes

Generated by 🚫 dangerJS against e041e1c

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.

We're missing a box-shadow on the thumb.


On a related note, I recall a discussion with @mbrookes about the classes API: "Slider track and rail means the same thing". My imagination fails to find something better.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 5, 2020
@@ -39,8 +40,9 @@ function ValueLabel(props) {
className: clsx(children.props.className),
},
<Root className={clsx(classes.offset, className)} theme={theme}>
<span className={classes.circle}>
<span className={classes.container}>
Copy link
Member

Choose a reason for hiding this comment

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

It seems like we removed and added classes but I see no documentation changed. Is this not something that is auto-generated or available in types?

/cc @mnajdova

Copy link
Member

Choose a reason for hiding this comment

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

These classes are not the props classes:

const classes = useValueLabelClasses(props);

Copy link
Member

@eps1lon eps1lon Oct 8, 2020

Choose a reason for hiding this comment

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

So there are no classnames that can target the ValueLabel component?

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.

I have noticed the following issues:

  1. Inversed color issue

Capture d’écran 2020-10-07 à 15 19 07

It's the 3rd regression from moving from the withStyles to styled API (emotion, not MD spec update related). I think that we didn't catch it in Argos because we introduced a new component, we didn't replace the existing one.

  1. No more box shadow when dragging

Capture d’écran 2020-10-07 à 15 05 46

  1. Vertical alignment issue, especially noticeable on a touch device

Capture d’écran 2020-10-07 à 15 06 13


On a related note, I wonder about the dense mode. What are your thoughts on providing a dense variant of the slider? The new design looks great on mobile. But on desktop? 20px, that's bold for the thumb! When Ant design is 14px, the previous spec was 12px, native slider on Chrome is 16px.

Examples of slider on Sketch and Figma:
Capture d’écran 2020-10-07 à 15 34 42
Capture d’écran 2020-10-07 à 15 34 07

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 10, 2020
@joshwooding
Copy link
Member Author

joshwooding commented Oct 10, 2020

I have noticed the following issues:

  1. Inversed color issue
Capture d’écran 2020-10-07 à 15 19 07

It's the 3rd regression from moving from the withStyles to styled API (emotion, not MD spec update related). I think that we didn't catch it in Argos because we introduced a new component, we didn't replace the existing one.

Inverted looks fine to me? It's broken in production only - locally it's fine. After rebasing on next it has seemed to fix itself.

  1. No more box shadow when dragging
Capture d’écran 2020-10-07 à 15 05 46

Box shadow on the thumb is used to provide the ripple, this will require a change there.

  1. Vertical alignment issue, especially noticeable on a touch device
Capture d’écran 2020-10-07 à 15 06 13

I'll look into it. This seems like a bug in how HD screens are handled via the browser. Everything seems to be 1 pixel (0.5px) off.

@mbrookes
Copy link
Member

https://github.com/mui-org/material-ui-pickers/issues/2136#issuecomment-706066008

Could we apply the same logic here? I'm not convinced that all of the changes to the slider in the current spec are an improvement.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 10, 2020

@mbrookes Google has started to use the new design. Happy to wait for Android 12 too.

Is this statement accurate? In the early days, Material Design guidelines were release after they deployed the design. It seems to be increasing going in the other direction.

I'm personally more in favor of implementing the design spec after it gains mass adoption in Google's product. This has the following advantages:

  • Production is production, the environment provides the strongest constraints. You won't see all the fail attempts.
  • The more used a UI/UX is, the more past learnings it creates in end-users. Making the app look more intuitive and look nicer.

On a related note, this makes me think of a discussion with the Material Design team. I recall they tried to create a distinction between "Google Material Design theme" and the "default Material Design theme". They encouraged us to follow the "default Material Design theme", but shouldn't we aim for "Google Material Design theme"?

@oliviertassinari

This comment has been minimized.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 11, 2020
@oliviertassinari oliviertassinari added the on hold There is a blocker, we need to wait label Oct 17, 2020
@oliviertassinari oliviertassinari removed the on hold There is a blocker, we need to wait label Nov 13, 2020
@oliviertassinari
Copy link
Member

The poll on Twitter seems to suggest that we can move forward. However, I would raise the need for a dense mode. I think that it's important for work tools. For instance, I was just checking a design tool's slider.

@oliviertassinari
Copy link
Member

I'm closing as the effort has been stale for some time, thanks for the exploration.

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 PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Slider] Update to match the specification
6 participants