[slider] Port component by epodivilov · Pull Request #11040 · mui/material-ui

epodivilov · 2018-04-16T20:15:35Z

Added implementation of the Slider component according to the Google guidelines

PS: Something happened to my previous PR so I created a new one.

Closes #4793

NayanSrivastav · 2018-04-20T16:41:22Z

Thank @mbrookes for this, when can we expect this merge?

epodivilov · 2018-04-27T16:21:16Z

@mbrookes what about this pull request?

mbrookes · 2018-04-27T17:30:46Z

@epodivilov My task for this weekend.

mbrookes · 2018-04-28T16:31:50Z

Okay, this is starting to take shape!

Keyboard input in the basic slider is 💯👌 , however mouse / touch input has the same granularity (1% step) as keyboard input, rather than being continuous, which gives a course feel to the movement. v0 had the same issue, so it would be good to address that here. It isn't a show-stopper, but I fear if we don't fix it now, it might get left like this.

Since you're using ButtonBase for the thumb, it would be preferable to use the focus ripple when keyboard focussed rather than a custom style.

The vertical slider seems backwards - I think the default should be from bottom-to-top, with reverse being top-to-bottom.

@Pajn pointed out that the use of top, left etc. for positioning is causing a performance hit. Did you have an opportunity to look at using transform: translate() and transform: scale()?

The testsh ave been renamed from .spec.js to .test.js

Once we get these last bits sorted, we can dig into code review (I have been focussing mostly on functionality).

Thanks for all your hard work! 👏

epodivilov · 2018-04-29T16:07:30Z

@mbrookes Thanks for review.
First of all I would like to note that the ripple effect is not according to the google guidelines.
About performance issue I will try to check.

About mouse / touch behavior I have a question. Right now I round value to step. if you do not do this, then which sign is worth rounding? For example: I clicked roughly in the middle of the slider and got value 0.5987654321. Now I will return the value 0.59. But it same value, which you can get by keyboard.

mbrookes · 2018-04-29T22:05:54Z

@epodivilov You're right, no ripple in the spec, or the MCW (or other) implementations. I'm a little surprised at that, as the ripple is designed as a visual affordance to make the focused component more obvious.

Regarding rounding the value - MCW don't round the value returned, so the developer can decide what precision to apply: https://material-components-web.appspot.com/slider.html

However unlike MCW, I believe we should round aria-valuenow to a sensible level of precision (3 digits?), so that a screen-reader user isn't potentially faced with an unending list of spoken digits if the screen-reader doesn't truncate it.

mbrookes · 2018-04-29T22:11:24Z

One other thing you can do as part of this PR is to update the Supported Components page. You can follow the SpeedDial example, but I think it would be a good idea to add the checkmark to both Slider and SpeedDial.

mbrookes · 2018-05-09T11:51:17Z

@epodivilov are you still working on this? It feels like we're very close to final review.

epodivilov · 2018-05-11T09:05:11Z

@mbrookes Yes, I still working on this PR. I had a small vacation. I'll try to fix it this weekend.

mbrookes · 2018-05-11T14:13:32Z

Thanks @epodivilov Hope you had a nice break! ☺️

epodivilov · 2018-05-11T20:57:26Z

@mbrookes I fixed rounding of values and updated the Supported Components page. Please, review last updates

mbrookes · 2018-05-11T21:32:01Z

@epodivilov Looks like something's gone wrong with the branch. You don't need to create a new PR, just clean up the brach and force push.

mbrookes

This is looking great! Thanks for working on it.

mbrookes · 2018-05-11T21:40:15Z

+import classNames from 'classnames';
+import withStyles from '../../../material-ui/src/styles/withStyles';
+import ButtonBase from '../../../material-ui/src/ButtonBase';
+import { fade } from '../../../material-ui/src/styles/colorManipulator';


These should be imported as:

import withStyles from 'material-ui/styles/withStyles';

etc.

Thought we fixed this already, but hey! 😄

mbrookes · 2018-05-11T21:40:39Z

+import withStyles from '../../../material-ui/src/styles/withStyles';
+import ButtonBase from '../../../material-ui/src/ButtonBase';
+import { fade } from '../../../material-ui/src/styles/colorManipulator';
+import clamp from '../utils/clamp';


I meant for this to be in the core material-ui so we can reuse it there, but I can fix it up later.

epodivilov · 2018-05-12T13:28:20Z

@mbrookes What does you mean "clean up the branch"? Like this https://stackoverflow.com/a/25357146 ?

Pajn · 2018-05-12T13:39:34Z

I would personally rebase and squash but that is of coursee up to you what you are most comfortable with. The end result is a nice simple commit on top of the current v1-beta in both cases :)

mbrookes · 2018-05-12T14:20:05Z

@epodivilov it wasn't about squashing (we can do that at merge), but there were somehow a bunch of unrelated commits in the PR. I wasn't sure if a rebase would be sufficient, or whether you'd have to cherry-pick your commits back onto a clean branch so "clean up" meaning whatever it needed to get it back into a reviewable, mergeable state.

goto-bus-stop · 2018-05-13T10:36:31Z

+import PropTypes from 'prop-types';
+import classNames from 'classnames';
+import withStyles from '@material-ui/core/styles/withStyles';
+import ButtonBase from '@material-ui/core//ButtonBase';


core//ButtonBase → core/ButtonBase

goto-bus-stop · 2018-05-17T08:38:34Z

+          onMouseDown={this.handleMouseDown}
+          onTouchStartCapture={this.handleTouchStart}
+          onTouchMove={this.handleMouseMove}
+          onKeyboardFocus={this.handleFocus}


onKeyboardFocus was renamed to onFocusVisible in #11188

goto-bus-stop · 2018-05-17T08:49:40Z

Just tried this again with @material-ui/[email protected], works great! (except for the onKeyboardFocus thing) Thanks for the work!

epodivilov · 2018-05-19T05:11:19Z

@goto-bus-stop Great news. Should I fix something in my code, or do you fix it yourself? And when you plan to merge the PR?

goto-bus-stop · 2018-05-19T08:41:17Z

Oh I don't know, I'm not a maintainer, just a user excited for this PR 😛

mbrookes · 2018-05-19T08:52:07Z

@epodivilov Yes please, if you could rename that prop as @goto-bus-stop pointed out, that would be great! Sorry I missed it.

…aterial-ui into slider-v2

oliviertassinari

I just had a look at the source code. Thanks for taking the time to work on i!

oliviertassinari · 2018-05-21T17:15:13Z

@@ -0,0 +1,3 @@
+export default function clamp(value, min = 0, max = 100) {


Can potentially be shared at some point with colorManipulator.

That was the original intention - we have a clamp function in a couple of places. I plan to consolidate them once this is merged.

oliviertassinari · 2018-05-21T17:18:14Z

+      vertical,
+      reverse,
+      disabled,
+    } = this.props;


It would be great to spread the other properties on the root element.

oliviertassinari · 2018-05-21T17:20:51Z

+          onBlur={this.handleBlur}
+          onKeyDown={this.handleKeyDown}
+          onMouseDown={this.handleMouseDown}
+          onTouchStartCapture={this.handleTouchStart}


Why do we need to trigger the event during the capture phase? It's the first time we do such.

I need to prevent page scrolling when moving the slider (function preventPageScrolling).I should catch event before preventing, so I use capture phase.

oliviertassinari · 2018-05-21T17:21:44Z

+        aria-orientation={vertical ? 'vertical' : 'horizontal'}
+        onClick={this.handleClick}
+        ref={node => {
+          this.container = findDOMNode(node);


The default component is a div. I don't think that you need to call findDOMNode.

To prevent page scrolling, I need to catch touch events and prevent them from entering outside of the container. So I need get a link to container component.

oliviertassinari · 2018-05-21T17:22:31Z

+  };
+};
+
+const KEY_CODES = {


This is already packages into the keycode dependency. No need for duplicating it.

You can find many examples in the codebase.

Replaced that code to keycode library

oliviertassinari · 2018-05-21T17:23:33Z

+  };
+
+  return {
+    /* Styles for wrapper container */


I wish we will be able to extra these comments and add it to the API documentation automatically 💯 at some point.

I'm not understand, should I remove this comments?

oliviertassinari · 2018-05-21T17:23:58Z

+      margin: '10px 0',
+      padding: '6px 0',
+      cursor: 'pointer',
+      '-webkit-tap-highlight-color': 'transparent',


WebkitTapHighlightColor: 'transparent',

For consistency, like we need everywhere else.

oliviertassinari · 2018-05-21T17:25:11Z

+  ARROW_DOWN: 40,
+};
+
+function addEventListener(node, event, handler, capture) {


the capture argument is never used.

By the way. I'm not sure you need this helper function.

This function is used to detect movement of the mouse outside the component, as well as to detect the termination of interaction with the component. So I can remove unused argument, but I think is not a problem. If this function move to all helpers function somebody might need this parameter.

oliviertassinari · 2018-05-21T17:26:28Z

+  };
+}
+
+const calculatePercent = (node, event, isVertical, isReverted) => {


You are using a function with the other helpers, not an arrow, maybe we should be consistent.

mbrookes · 2018-05-24T21:50:44Z

@oliviertassinari Any last comments?

bennidhamma · 2018-05-24T23:45:53Z

+    this.globalMouseUpListener = addEventListener(document, 'touchend', this.handleMouseUp);
+
+    if (typeof this.props.onDragEnd === 'function') {
+      this.props.onDragStart(event);


Do you mean to test for onDragEnd here? Or onDragStart?

bennidhamma · 2018-05-24T23:46:02Z

+    this.globalMouseMoveListener = addEventListener(document, 'mousemove', this.handleMouseMove);
+
+    if (typeof this.props.onDragEnd === 'function') {
+      this.props.onDragStart(event);


Do you mean to test for onDragEnd here? Or onDragStart?

oliviertassinari · 2018-05-25T16:54:47Z

@mbrookes None :)

mbrookes · 2018-05-25T18:00:13Z

@epodivilov Thanks for all your hard work, and sticking with it! An amazing contribution for your first!

epodivilov mentioned this pull request Apr 16, 2018

[slider] Port component #10665

Closed

oliviertassinari added scope: slider Changes related to the slider. package: lab Specific to the lab. labels Apr 16, 2018

mbrookes self-requested a review April 16, 2018 21:44

mbrookes reviewed May 11, 2018

View reviewed changes

Added Slider component

cfeafac

Merge branch 'v1-beta' into slider-v2

49eea4e

oliviertassinari changed the base branch from v1-beta to master May 12, 2018 18:20

mbrookes added 2 commits May 13, 2018 10:20

Remove an extra blank line from supported-component.md

f01e541

[Slider] Fix the imports

6269aa2

mbrookes requested a review from oliviertassinari May 13, 2018 10:07

mbrookes approved these changes May 13, 2018

View reviewed changes

goto-bus-stop reviewed May 13, 2018

View reviewed changes

Fix oops

6a33588

goto-bus-stop reviewed May 17, 2018

View reviewed changes

AhmetTanakol mentioned this pull request May 17, 2018

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

Closed

Подивилов Евгений and others added 6 commits May 19, 2018 17:21

Added Slider component

2aa76b0

Remove an extra blank line from supported-component.md

f93b230

[Slider] Fix the imports

2ab656a

Fix oops

e3979a3

Merge branches 'slider-v2' and 'slider-v2' of github.com:epodivilov/m…

69f005d

…aterial-ui into slider-v2

Renamed property onKeyboardFocus

c0febca

mbrookes approved these changes May 19, 2018

View reviewed changes

oliviertassinari reviewed May 21, 2018

View reviewed changes

Fixed issues from comments

9850b4e

bennidhamma reviewed May 24, 2018

View reviewed changes

Fixed callbacks

27808b8

mbrookes merged commit 0b63c30 into mui:master May 25, 2018

This comment has been minimized.

Sign in to view

oliviertassinari mentioned this pull request Sep 23, 2018

[slider] Replace reversed with rtl support on horizontal sliders #12972

Merged

1 task

oliviertassinari added type: new feature Expand the scope of the product to solve a new problem. and removed package: lab Specific to the lab. labels Jan 9, 2021

oliviertassinari changed the title ~~[Slider] Port component~~ [slider] Port component May 9, 2026

		@@ -0,0 +1,3 @@
		export default function clamp(value, min = 0, max = 100) {

Uh oh!

Uh oh!

Conversation

epodivilov commented Apr 16, 2018 • edited by oliviertassinari Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NayanSrivastav commented Apr 20, 2018

Uh oh!

epodivilov commented Apr 27, 2018

Uh oh!

mbrookes commented Apr 27, 2018

Uh oh!

mbrookes commented Apr 28, 2018

Uh oh!

epodivilov commented Apr 29, 2018

Uh oh!

mbrookes commented Apr 29, 2018

Uh oh!

mbrookes commented Apr 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbrookes commented May 9, 2018

Uh oh!

epodivilov commented May 11, 2018

Uh oh!

mbrookes commented May 11, 2018

Uh oh!

epodivilov commented May 11, 2018

Uh oh!

mbrookes commented May 11, 2018

Uh oh!

mbrookes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epodivilov commented May 12, 2018

Uh oh!

Pajn commented May 12, 2018

Uh oh!

mbrookes commented May 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goto-bus-stop May 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goto-bus-stop commented May 17, 2018

Uh oh!

epodivilov commented May 19, 2018

Uh oh!

goto-bus-stop commented May 19, 2018

Uh oh!

mbrookes commented May 19, 2018

Uh oh!

oliviertassinari left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epodivilov commented Apr 16, 2018 •

edited by oliviertassinari

Loading

mbrookes commented Apr 29, 2018 •

edited

Loading

mbrookes commented May 12, 2018 •

edited

Loading

goto-bus-stop May 13, 2018 •

edited

Loading