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] removed react-draggable dependency (fixes IE!) #1825

Merged
merged 10 commits into from
Oct 15, 2015

Conversation

igorbt
Copy link
Contributor

@igorbt igorbt commented Oct 7, 2015

Fixing this bug #708, I ended up that it's better to entirely remove react-draggable dependency. With this change I saw also it seems to be more responsive.
Also, this fixes desynchronization between the mouse position and slider handle.

A (very) little breaking change in the onDragStop and onDragStart APIs - it no longer provides second argument - ui. I think it was useless anyway because it had handle offset in pixels. I cannot think about a scenario when someone really need this. I you don't agree, I can try to add it back by calculating it.

@igorbt
Copy link
Contributor Author

igorbt commented Oct 8, 2015

Fixes #1745 - see #1745 (comment).

@igorbt
Copy link
Contributor Author

igorbt commented Oct 9, 2015

@yongxu, @shaurya947, @KapJI can you please take a look here. I saw you are the last contributors to the Slider component, so maybe you can make a quick review.
The thing is that we use Slider in our product and we had a lot of issues with it (and material-ui issue tracker also if full of slider bugs) and this PR fixed all of them for us.

@KapJI
Copy link
Contributor

KapJI commented Oct 9, 2015

I reviewed this PR and tested it locally on the latest Chrome, Safari and Firefox on OS X 10.11. Now handle and cursor don't diverge. Seems good to me. Good job, @igorbt!

@KapJI KapJI mentioned this pull request Oct 9, 2015
let value = this._alignValue(this._percentToValue(percent));
if (this.props.onChange) this.props.onChange(e, value);
this.setPercent(percent, () => {
if (this.props.onChange) this.props.onChange(e, this.state.value);
Copy link
Contributor

Choose a reason for hiding this comment

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

This will call onChange many times with same value during dragging. I think it will be better to call onChange only when value changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @KapJI for review and for this finding. You are right and I just fixed it, but from setPercent, it seemed more convenient there for me.

@igorbt
Copy link
Contributor Author

igorbt commented Oct 10, 2015

@yongxu, thanks for review!
You are right, percent is not always the same as this.state.percent. This is why in setState passed value is normalized before putting in state:

setPercent(percent, callback) {
    let value = this._alignValue(this._percentToValue(percent));
    let { min, max } = this.props;
    let alignedPercent = (value - min) / (max - min);
    this.setState({value: value, percent: alignedPercent}, callback);
},

@KapJI
Copy link
Contributor

KapJI commented Oct 10, 2015

@yongxu comparing percent difference with step is not a good idea, because step is measured in absolute value, not in percent.

so avoiding unnecessary re-rendering and unnecessary calling of onChange from _updateWithChangeEvent.
@yongxu
Copy link
Contributor

yongxu commented Oct 10, 2015

Thanks @KapJI , you were right I didn't think about it. it should be something like(step/(max-min))
@igorbt have solved this in another way, I think it should be good enough!

@igorbt
Copy link
Contributor Author

igorbt commented Oct 10, 2015

@yongxu you are right, just removed that line.

@igorbt
Copy link
Contributor Author

igorbt commented Oct 13, 2015

@shaurya947 can you please also have a look and merge this PR?

@igorbt
Copy link
Contributor Author

igorbt commented Oct 15, 2015

@shaurya947, I just rebased. Can you please also have a look and merge this PR? That's a pain to keep rebasing given the change rate in the library.

shaurya947 added a commit that referenced this pull request Oct 15, 2015
[Slider] removed react-draggable dependency (fixes IE!)
@shaurya947 shaurya947 merged commit 1e0a4a5 into mui:master Oct 15, 2015
@shaurya947
Copy link
Contributor

There you go, thanks @igorbt

@igorbt
Copy link
Contributor Author

igorbt commented Oct 15, 2015

Awsome! Thanks @shaurya947! I think at least 5 bugs shoul be closed now.

@igorbt igorbt deleted the slider-fix branch October 15, 2015 19:53
@zannager zannager added the component: slider This is the name of the generic UI component, not the React module! label Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: slider This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants