[Slider] Port component #10665

epodivilov · 2018-03-15T13:05:03Z

Added implementation of the Slider component according to the Google guidelines

mbrookes · 2018-03-15T21:41:19Z

@epodivilov Looks like a great start!

We are putting newly created / migrated components in packages/material-ui-lab. Please could you move this to there? Also, there are some stray Windows CRs, including in unrelated files. Could you take a look?

epodivilov · 2018-03-16T13:31:54Z

@mbrookes Thanks. Will be done!

Maybe worth adding information about packages/material-ui-lab to the file CONTRIBUTING.md?

mbrookes · 2018-03-16T20:17:41Z

@epodivilov Good point. Done!

mbrookes

Please could you move the demos under docs/src/pages/lab?

epodivilov · 2018-03-19T17:37:51Z

@mbrookes Ok. Done.

mbrookes

Sorry, was too hasty in my last review.

mbrookes · 2018-03-26T12:18:22Z

docs/src/pages/lab/slider/ReverseSlider.js

+import React from 'react';
+import PropTypes from 'prop-types';
+import { withStyles } from 'material-ui/styles';
+import Slider from '../../../../../packages/material-ui-lab/src/Slider/Slider';


You can use import Slider from '@material-ui/lab/Slider' across the demos.

mbrookes · 2018-03-26T12:19:40Z

docs/src/pages/lab/slider/slider.md

+A [slider](https://material.io/guidelines/components/sliders.html) is an interface for users to input a value in a range. Sliders can be continuous or discrete and can be enabled or disabled.
+
+## Simple slider
+{{"demo": "pages/demos/slider/SimpleSlider.js"}}


paths need to be updated

mbrookes · 2018-03-26T12:20:46Z

pages/lab/slider.js

+    <MarkdownDocs
+      markdown={markdown}
+      demos={{
+        'pages/demos/slider/SimpleSlider.js': {


paths need to be updated

mbrookes · 2018-03-26T12:27:20Z

We also need to work on the ARIA story:

epodivilov · 2018-03-26T17:53:18Z

@mbrookes Ok, I updated paths
What do you mean " to work on the ARIA story"? Should I support all hotkey?
Right now Slider support changing value by arrow keys.

mbrookes · 2018-03-26T18:22:03Z

packages/material-ui-lab/src/Slider/Slider.js

+import withStyles from '../../../../src/styles/withStyles';
+import ButtonBase from '../../../../src/ButtonBase';
+import addEventListener from '../../../../src/utils/addEventListener';
+import { fade } from '../../../../src/styles/colorManipulator';


These should import from material-ui.

epodivilov · 2018-03-26T18:23:44Z

@mbrookes And I can't understand, why happend "Deploy preview failed."

Could you explain the order of adding a new component? Given the location of this component in the lab?

mbrookes · 2018-03-26T18:24:53Z

@epodivilov I was thinking more of the ARIA attributes (Roles, States, and Properties) - as much as possible this should be handled by the component, however it looks like we should also support home / end key at least. (Page up / down is optional).

mbrookes · 2018-03-26T18:29:50Z

And I can't understand, why happened "Deploy preview failed."

Not sure, we're just experimenting with Netlify. You could try rebasing off v1-beta.

Could you explain the order of adding a new component?

Sorry, not sure I understand the question?

epodivilov · 2018-03-26T18:41:50Z

Ok, I will add support home / end keys.
Also, I will add "aria-valuenow", "aria-valuemin", "aria-valuemax", "aria-orientation" and role "slider" directly to the component. And other aria-* options user can add independently. Is it ok?

And about my previous question.
Can you explain by steps how to add a component to the project? It would be nice if this information was in the file CONTRIBUTING.md

For example:

You should create new component into ./packages/material-ui-lab/src
To build demo page you should run command ...
To build api page you should run command ...
and etc.

Right now I`m not shure, that all my changes is needed. For example changes in modules/components/withRoot.js

mbrookes · 2018-03-26T18:53:44Z

And other aria-* options user can add independently. Is it ok?

Thanks for adding those. 👍 It would be ideal if you could also label the sliders in the examples, and show the correct usage of aria-labelledby to link the label and slider.

mbrookes · 2018-03-26T18:57:43Z

@epodivilov Yes, you're right, the contributing guide hasn't kept up with changes to the project. I'll take a look. The main omission is think is the yarn docs:api / npm run docs:api script; everything else you changed has to be dome manually.

mbrookes · 2018-03-26T19:31:15Z

Just noticed that the description for min and max aren't correct. (I realised they were copied from v0, and see that they aren't correct there either!). You could drop the words " on a scale from 0 to 1 inclusive".

Also, perhaps "Cannot be equal to min." should be "Should not be equal to min."? (It can, but it shouldn't...)

mbrookes · 2018-03-28T00:34:16Z

@epodivilov Thanks for following up so quickly on the changes - at this rate it's going to be ready for v1 rather than lab! 😄

I'm snowed with work at the mo, but I promise to give it a proper review next week. @ me if I haven't checked in.

Aubron · 2018-04-05T13:46:36Z

Hello @mbrookes!

(I know, it's thursday, I'm not the person who authored this PR, etc etc. I'm just a poor frontend developer with a deadline coming up who would rather use a material-ui solution than adding and customizing rc-slider to spec. 🤷‍♂️ )

mbrookes · 2018-04-05T17:26:48Z

I hadn't forgotten, I promise. 😇

@Aubron If you wanted to help in the meantime, you can take a look at it here and test it out.

@epodivilov A couple of issues I was going to dig into that you could take a look at:

In Chrome (MacOS), the Thumb gains keyboard focus after being clicked.
In Chrome and Firefox, there's a lag between sliding one slider, and the track update of the other slider in the Vertical and Reverse examples if you move the slider quickly. (The second Thumb) moves before the track changes I know sliders wouldn't normally be linked like this, but it does suggest some possible underlying issue.
You could use the Typography component for the label.

goto-bus-stop · 2018-04-06T09:00:00Z

Thanks for working on this component @epodivilov! I copy pasted this into a project some days ago because it was the last missing 1.0 component :). I noticed one issue as well:

When dragging the Slider all the way to either end with the mouse, the value can exceed the min/max range. I think a clamp is missing on the event position.

I added a normalizeValue call around the percent assignment here to fix; it may not be the best solution but it works for my purpose 🤷‍♀️

epodivilov · 2018-04-06T18:43:57Z

@mbrookes I made some changes. But I`m not understood issue about focus. It is standard behavior of element.

Look at the example with the standard HTML component https://codepen.io/anon/pen/jzQEJP

@goto-bus-stop Thanks for the feedback 👍 With your help I found a couple of bugs and fixed them.

Pajn · 2018-04-06T19:16:03Z

Some things I noticed by trying https://deploy-preview-10665--material-ui-next.netlify.com/lab/slider/

The slider feels quite slugish and the thumb drags behind the pointer. This is due to the transition, if I disable it the slider thumb follows the pointer much better. I think this should be disabled while dragging a non-step slider with a mouse or touch but still be enabled for keyboard and all interactions with a step slider.
The "non" step slider goes in steps of 10%.
I assume this can be configured by setting a much smaller step size for applications that require that but that would make keyboard usage a pain. Usually sliders without a real or very small step size can be set precisely with the mouse but goes in bigger steps with the keyboard. Maybe an additional property for keyboard step size could be added with defaults being 0.01 for mouse/touch step size and 0.1 for keyboard step size, however if only "step" is set keyboard step size should default to that instead to avoid unexpected/surprising behaviour.
Even on my laptop dragging the slider causes a noticeable purple layout cost in chromes devtools and by emulating the performance of my phone it is even dropping frames and has a median of 30FPS. This is caused by the use of top, left, right and bottom for positioning the thumb as well as width/height for the track. Could this be replaced with transform: translate() and transform: scale() for a higher performance that would stay at 60FPS even on the phone?
Note that this is on the quite simple demo page, layout is in almost all cases done on the complete page so when placed on a more complex page the cost would be even higher and we could start dropping frames even on laptops!
I recently did a performance analysis for a customer with a very complex page that did have recalculate style and layout each taking 200-250ms each on my laptop. That is sure extreme but even something nearing that would make the slider extremely sluggish. Avoiding layout also avoid such problems.

Thank you for working on this!
I know text easily sounds harsh on the internet, that is not at all my intention. You are off to a great start and this is just suggestions that I think will help you improve it even more.

mbrookes · 2018-04-06T19:20:39Z

docs/src/pages/lab/slider/DisabledSlider.js

+import { withStyles } from 'material-ui/styles';
+import Slider from '@material-ui/lab/Slider';
+
+const styles = () => ({


No need for the function.

mbrookes · 2018-04-06T19:21:18Z

docs/src/pages/lab/slider/DisabledSlider.js

+    <div className={classes.container}>
+      <Slider value={0} min={0} max={6} disabled />
+      <Slider value={3} min={0} max={6} disabled />
+      <Slider value={6} min={0} max={6} disabled />


No need to include props that match the defaults (min).

mbrookes · 2018-04-06T19:39:53Z

packages/material-ui-lab/src/Slider/Slider.js

+    global.addEventListener('touchstart', this.touchStart, { passive: false });
+  }
+
+  componentWillReceiveProps(nextProps) {


Could you update this for React 16.3? We're using the polyfill for the new lifecycle methods to support earlier versions.

mbrookes · 2018-04-06T19:46:08Z

packages/material-ui-lab/src/Slider/Slider.js

+  END: 35,
+};
+
+function normalizeValue(value, min = 0, max = 100) {


We call this 'clamp' in other parts of the code (CircularProgress, and colorManipulator, where it also gives a warning of out-of-range). I wonder if it should move to utils as a shared function?

mbrookes · 2018-04-06T19:54:05Z

packages/material-ui-lab/src/Slider/Slider.js

+  };
+}
+
+function normalizeValue(value, min = 0, max = 100) {


We call this clamp elsewhere (CircularProgress and colorManipulator, where it also gives a warning for out-of-range values). I wonder if we should move it to utils as a shared function?

Looking at the implementation of this vs CircularProgress, that seems reasonable.

Where I should place function? In /src/utils or in /packages/material-ui-lab/src/utils ?

mbrookes · 2018-04-06T20:32:02Z

I`m not understood issue about focus.

When you tab focus the Slider with the keyboard, it gets a focus indicator:

(Ideally that would be the pulsating focusRipple, rather than static, but lets come back to that).

In Chrome, when you click the Thumb, then "on mouse up" the keyboard focus is being applied - the focus indicator appears, even though there was no keyboard interaction.

mbrookes · 2018-04-07T17:10:38Z

The "non" step slider goes in steps of 10%.

Ouch! I didn't even spot this until I disabled the transition.

For the continuous slider, MCW is using 1% for up / down, and 4% for page-up / page-down for keyboard input, while mouse granularity is effectively determined by the screen resolution. Those seem like sensible defaults.

v0.x is also using 1% for up/down, but doesn't support page up / down.

Even on my laptop dragging the slider causes a noticeable purple layout cost

Out of idle curiosity, I replaced the track with a restyled <LinearProgress variant="determinant"> with transition animations disabled. this uses scaleX() and it's as snappy as a snappy thing. The current Thumb, not so much. 😄

mbrookes · 2018-04-07T17:34:55Z

One more thing: The slider should continue to slide even when the mouse is outside the container.

mbrookes · 2018-04-07T17:40:28Z

packages/material-ui-lab/src/Slider/Slider.js

+    secondary: '#bdbdbd',
+    focused: '#9e9e9e',
+    disabled: '#bdbdbd',
+  };


These colours need to be taken from the theme so users can modify them - either directly from theme.palette.grey, or colorManipulator'd version of them.

epodivilov · 2018-04-16T19:50:41Z

@mbrookes Something happened to my previous PR so I created a new one #11040

mbrookes · 2018-04-16T22:00:49Z

@epodivilov Thanks for persevering with it! See you on #11040 👍

oliviertassinari added new feature New feature or request package: lab Specific to @mui/lab labels Mar 15, 2018

mbrookes added component: slider This is the name of the generic UI component, not the React module! and removed new feature New feature or request labels Mar 19, 2018

mbrookes requested changes Mar 19, 2018

View reviewed changes

mbrookes approved these changes Mar 19, 2018

View reviewed changes

mbrookes requested changes Mar 26, 2018

View reviewed changes

mbrookes reviewed Mar 26, 2018

View reviewed changes

gregnb mentioned this pull request Mar 31, 2018

Feature Request: sliders in dropdown gregnb/mui-datatables#18

Closed

mbrookes requested changes Apr 6, 2018

View reviewed changes

mbrookes reviewed Apr 6, 2018

View reviewed changes

mbrookes reviewed Apr 7, 2018

View reviewed changes

AhmetTanakol mentioned this pull request Apr 10, 2018

Port Slider Renderer to material-ui eclipsesource/jsonforms#853

Closed

Implemented slider component

f6ac9db

epodivilov closed this Apr 16, 2018

oliviertassinari added new feature New feature or request and removed package: lab Specific to @mui/lab labels Jan 9, 2021

[Slider] Port component #10665

[Slider] Port component #10665

Conversation

epodivilov commented Mar 15, 2018 • edited by mbrookes

mbrookes commented Mar 15, 2018

epodivilov commented Mar 16, 2018

mbrookes commented Mar 16, 2018

mbrookes left a comment

Choose a reason for hiding this comment

epodivilov commented Mar 19, 2018

mbrookes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrookes commented Mar 26, 2018 • edited

epodivilov commented Mar 26, 2018

Choose a reason for hiding this comment

epodivilov commented Mar 26, 2018

mbrookes commented Mar 26, 2018

mbrookes commented Mar 26, 2018

epodivilov commented Mar 26, 2018

mbrookes commented Mar 26, 2018

mbrookes commented Mar 26, 2018

mbrookes commented Mar 26, 2018 • edited

mbrookes commented Mar 28, 2018

Aubron commented Apr 5, 2018

mbrookes commented Apr 5, 2018 • edited

goto-bus-stop commented Apr 6, 2018 • edited

epodivilov commented Apr 6, 2018

Pajn commented Apr 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrookes Apr 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrookes commented Apr 6, 2018 • edited

mbrookes commented Apr 7, 2018 • edited

mbrookes commented Apr 7, 2018

Choose a reason for hiding this comment

epodivilov commented Apr 16, 2018 • edited

mbrookes commented Apr 16, 2018

epodivilov commented Mar 15, 2018 •

edited by mbrookes

mbrookes commented Mar 26, 2018 •

edited

mbrookes commented Mar 26, 2018 •

edited

mbrookes commented Apr 5, 2018 •

edited

goto-bus-stop commented Apr 6, 2018 •

edited

mbrookes Apr 6, 2018 •

edited

mbrookes commented Apr 6, 2018 •

edited

mbrookes commented Apr 7, 2018 •

edited

epodivilov commented Apr 16, 2018 •

edited