Bugfix/keep pushable #304

Robin-front · 2017-08-10T03:46:12Z

trace: #159
when check the values in componentWillReceiveProps function, you use map but no index or Corresponding handle. The handle will be always what you are move.
Then it invoke ensureValueNotConflict function, but ensureValueNotConflict just return prev or next handle value. (I think it should in consideration of thershold)

So the gap of pushable not work when allowCross=false and set the value props (to update the display at each state)

I just do it. but maybe someone has more elegant code. Many places invoke trimAlignValue func, so it may not index and nextProps args sometimes.

base issue #159 , after fixed, I make some examples：
all examples I set pushable={10} and allowCross={false}

coveralls · 2017-08-10T03:48:51Z

Coverage increased (+0.3%) to 62.069% when pulling dd6c61a on Robin-front:bugfix/keep-pushable into caf5866 on react-component:master.

paranoidjk · 2017-09-15T02:54:18Z

@Robin-front Could you add some test case about the problem you want to fix?

coveralls · 2017-09-18T03:57:35Z

Coverage increased (+2.05%) to 63.793% when pulling 2f3e18a on Robin-front:bugfix/keep-pushable into caf5866 on react-component:master.

paranoidjk · 2017-10-26T03:03:35Z

close #347

paranoidjk · 2017-10-26T03:13:17Z

@Robin-front Could you pleasse rebase master? I add a Controlled Range, not allow across demo in example, and run with you fix, still got problem when handle i is pushed to i+1, then any of other handle being not moveable.

paranoidjk · 2017-10-26T03:15:56Z

src/Range.jsx

    const state = this.state || {};
-    const { handle, bounds } = state;
+    const { bounds } = state;
+    const handle = (i === undefined) ? state.handle : i;


In which case i could be undefiend?

yes, because ensureValueNotConflict invoked by trimAlignValue, then trimAlignValue was invoked many place.
sometimes, only trim current handle value ( didn't pass index, like calcValueByPos in createSlider.jsx ) , but others, maybe map all handle values to trim.

paranoidjk · 2017-10-26T03:17:11Z

src/Range.jsx

-    if (!allowCross && handle != null) {
-      if (handle > 0 && val <= bounds[handle - 1]) {
-        return bounds[handle - 1];
+    if (!allowCross && handle != null && bounds !== undefined) {


bounds shouldn't be undefined either ?

when init component, trimAlignValue invoked in constructor, at the time this.state is undefined, bounds also.
anyway, it must walk over this function to return origin val

paranoidjk · 2017-10-26T03:18:08Z

src/Range.jsx

-      if (handle > 0 && val <= bounds[handle - 1]) {
-        return bounds[handle - 1];
+    if (!allowCross && handle != null && bounds !== undefined) {
+      if (handle > 0 && val <= (bounds[handle - 1] + thershold)) {


Actually, handle is a array index, so it should always greater than 0

~~No, handle will be null only one moment when init component, constructor init state this.state = { handle: null, recent, bounds, };~~

~~once click any handle, handle will be no more null.~~

oops, sorry, I am mistaken about you.
Here should be judge safe distance of the handle and surrounding handles.
handle is index, handle <= bounds.length-1 && handle >= 0, with equal.

paranoidjk · 2017-10-26T03:19:30Z

src/Range.jsx

      }
-      if (handle < bounds.length - 1 && val >= bounds[handle + 1]) {
-        return bounds[handle + 1];
+      if (handle < bounds.length - 1 && val >= (bounds[handle + 1] - thershold)) {


same as below, handle < bounds.length - 1 should always be true.

Robin-front · 2017-10-26T03:47:10Z

@paranoidjk
Yes, It seems have any other problem after merge master.
Same with you, any of other handle being not moveable. look like focus the same handle again.
I will check.

Robin-front · 2017-10-26T09:20:10Z

@paranoidjk
The problem about handle cannot move is resolve already. The reason is I add extra mouseup event, so onEnd does not trigger.

And the CI test error below seems RangeWithTooltip error.
my local test cases are all passed.

paranoidjk · 2017-10-26T09:21:40Z

The onEnd bug should be fixed in 18d4fa3 already

Robin-front · 2017-10-26T09:36:54Z

Yes, but this mouseup function was add by another PR, I think it is redundant code now.

Because when call addMouseEvent, mouseup has add to ownerDocument already. It will trigger onEnd, then onEnd will removeEvent also.

paranoidjk · 2017-11-03T03:00:17Z

src/Range.jsx

@@ -261,23 +261,24 @@ class Range extends React.Component {
    return true;
  }

-  trimAlignValue(v, nextProps = {}) {
+  trimAlignValue(v, handle = this.state.handle, nextProps = {}) {


createSlider will call trimAlignValue too https://github.com/react-component/slider/search?utf8=%E2%9C%93&q=trimAlignValue&type=

Yes, but args handle & nextProps both optional. So, I think it dosen't matter.

paranoidjk · 2017-11-03T03:10:35Z

Add another demo of allowCross false && pushable true Range, both have bug right now 😅 9bbc8b1

coveralls · 2017-11-06T01:20:19Z

Coverage decreased (-1.1%) to 63.636% when pulling 8437ce8 on Robin-front:bugfix/keep-pushable into e296bd5 on react-component:master.

coveralls · 2017-11-06T02:24:42Z

Coverage decreased (-1.1%) to 63.636% when pulling de05632 on Robin-front:bugfix/keep-pushable into e296bd5 on react-component:master.

Robin-front · 2017-11-06T02:29:05Z

@paranoidjk
After merge master into my branch, it works well now. I can't reproduce.

coveralls · 2017-12-16T07:58:59Z

Coverage increased (+1.7%) to 74.609% when pulling 79c091e on Robin-front:bugfix/keep-pushable into db32618 on react-component:master.

Robin-front · 2017-12-16T08:23:50Z

@paranoidjk
Please checkout my lastest codes and review again.

trace: #226 #379

paranoidjk · 2017-12-16T11:20:23Z

@Robin-front Have you test the demo you added in this PR, pushable is still broken.

Robin-front · 2017-12-20T06:16:23Z

@paranoidjk
This is official online example, it's broken.

This is my pr, local example, it works well:

coveralls · 2017-12-20T06:20:25Z

Coverage increased (+5.9%) to 87.08% when pulling c4522e7 on Robin-front:bugfix/keep-pushable into c7f6f22 on react-component:master.

yesmeck · 2018-02-24T08:34:29Z

src/common/createSlider.jsx

-    onMouseUp = () => {
-      this.onEnd();
-      this.removeDocumentEvents();
-    }


Why is this callback removed? Will cause a memory leak if we don't remove listeners from the document.

Nevermind, listeners will be removed in onMouseDown.

No, not onMouseDown.
Actually, listeners will be removed in this.onEnd, when another mouseup event that bind on document was trigger.

So, mouseUp event bind on Slide is repeated with another mouseUp bind on document.(trace: #361 )

slider/src/common/createSlider.jsx

Line 161 in c7f6f22

this.onMouseUpListener = addEventListener(this.document, 'mouseup', this.onEnd);

slider/src/Range.jsx

Lines 111 to 115 in c7f6f22

onEnd = () => {

this.setState({ handle: null });

this.removeDocumentEvents();

this.props.onAfterChange(this.getValue());

}

yesmeck · 2018-02-24T08:37:38Z

@Robin-front Examples look good, could you rebase master, then I can merge it.

Robin-front added 3 commits August 9, 2017 18:55

keep pushable when allowCross is false

93d885f

correct handle in ensureValueNotConflict func

4f18aea

sorry for change origin example

dd6c61a

add test case

2f3e18a

Robin-front mentioned this pull request Oct 26, 2017

Controlled Range with allowCross=false has issues #347

Closed

Merge branch 'master' into bugfix/keep-pushable

6d7d607

paranoidjk reviewed Oct 26, 2017

View reviewed changes

Robin-front added 5 commits October 26, 2017 16:44

change variable index to handle for readable

227a126

add example for controlled component with allowCross false

cc060cd

add func name for lint

9f06b2d

extra mouseup event cause onEnd didnot trigger

d63b863

conflicts

3e74742

paranoidjk reviewed Nov 3, 2017

View reviewed changes

paranoidjk added the bug label Nov 3, 2017

Merge branch 'master' into bugfix/keep-pushable

8437ce8

add more test for pushable with not allowcross

de05632

Robin-front added 2 commits December 16, 2017 10:51

Merge branch 'rc/master' into bugfix/keep-pushable

c5322a8

update example about Controlled Range, not allow across, with pushable

79c091e

Robin-front mentioned this pull request Dec 16, 2017

Fixed issue where controlled Range sliders with allowCross=false and … #379

Closed

Robin-front added 2 commits December 20, 2017 14:15

update example

2743c4f

update test case

0d44182

paranoidjk requested a review from benjycui December 20, 2017 06:54

Robin-front added 2 commits February 6, 2018 13:24

Merge branch 'rc/master' into bugfix/keep-pushable

7c263a7

change the handle argv and testcase for clear logic

b0ffcc3

Robin-front mentioned this pull request Feb 6, 2018

Range slider issue, when min value is more than max value and also more than aria-valuemax value #395

Closed

yesmeck reviewed Feb 24, 2018

View reviewed changes

resolve conflict

c4522e7

yesmeck merged commit 7c82832 into react-component:master Feb 27, 2018

renovate bot mentioned this pull request Mar 21, 2018

Update rc-slider to ^8.0.0-abandoned sbardian/DebtTrackerAPI#14

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix/keep pushable #304

Bugfix/keep pushable #304

Robin-front commented Aug 10, 2017 •

edited

Loading

coveralls commented Aug 10, 2017

paranoidjk commented Sep 15, 2017

coveralls commented Sep 18, 2017

paranoidjk commented Oct 26, 2017

paranoidjk commented Oct 26, 2017

paranoidjk Oct 26, 2017

Robin-front Oct 26, 2017 •

edited

Loading

paranoidjk Oct 26, 2017

Robin-front Oct 26, 2017

paranoidjk Oct 26, 2017

Robin-front Oct 26, 2017 •

edited

Loading

Robin-front Oct 26, 2017

paranoidjk Oct 26, 2017

Robin-front commented Oct 26, 2017

Robin-front commented Oct 26, 2017

paranoidjk commented Oct 26, 2017 •

edited

Loading

Robin-front commented Oct 26, 2017 •

edited

Loading

paranoidjk Nov 3, 2017

Robin-front Nov 3, 2017

paranoidjk commented Nov 3, 2017 •

edited

Loading

coveralls commented Nov 6, 2017

coveralls commented Nov 6, 2017

Robin-front commented Nov 6, 2017

coveralls commented Dec 16, 2017

Robin-front commented Dec 16, 2017

paranoidjk commented Dec 16, 2017

Robin-front commented Dec 20, 2017 •

edited

Loading

coveralls commented Dec 20, 2017 •

edited

Loading

yesmeck Feb 24, 2018

yesmeck Feb 24, 2018

Robin-front Feb 26, 2018 •

edited

Loading

yesmeck commented Feb 24, 2018

	onEnd = () => {
	this.setState({ handle: null });
	this.removeDocumentEvents();
	this.props.onAfterChange(this.getValue());
	}

Bugfix/keep pushable #304

Bugfix/keep pushable #304

Conversation

Robin-front commented Aug 10, 2017 • edited Loading

coveralls commented Aug 10, 2017

paranoidjk commented Sep 15, 2017

coveralls commented Sep 18, 2017

paranoidjk commented Oct 26, 2017

paranoidjk commented Oct 26, 2017

Choose a reason for hiding this comment

Robin-front Oct 26, 2017 • edited Loading

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

Robin-front Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robin-front commented Oct 26, 2017

Robin-front commented Oct 26, 2017

paranoidjk commented Oct 26, 2017 • edited Loading

Robin-front commented Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paranoidjk commented Nov 3, 2017 • edited Loading

coveralls commented Nov 6, 2017

coveralls commented Nov 6, 2017

Robin-front commented Nov 6, 2017

coveralls commented Dec 16, 2017

Robin-front commented Dec 16, 2017

paranoidjk commented Dec 16, 2017

Robin-front commented Dec 20, 2017 • edited Loading

coveralls commented Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robin-front Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

yesmeck commented Feb 24, 2018

Robin-front commented Aug 10, 2017 •

edited

Loading

Robin-front Oct 26, 2017 •

edited

Loading

Robin-front Oct 26, 2017 •

edited

Loading

paranoidjk commented Oct 26, 2017 •

edited

Loading

Robin-front commented Oct 26, 2017 •

edited

Loading

paranoidjk commented Nov 3, 2017 •

edited

Loading

Robin-front commented Dec 20, 2017 •

edited

Loading

coveralls commented Dec 20, 2017 •

edited

Loading

Robin-front Feb 26, 2018 •

edited

Loading