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

Bug fix - do not drop previous constraint on click #4089

Merged
merged 3 commits into from
Jul 31, 2019

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Jul 30, 2019

Addressing https://github.com/plotly/phoenix-integration/issues/235
The bug was related to dropping the previous constraints on clicking new point to add to the list.
Noting that in that scenario the mousemove & drag functions were not called; this behaviour is fixed by calling them from dragend function.

Codepen before
Codepen after

@plotly/plotly_js

@archmoj archmoj added bug something broken status: reviewable labels Jul 30, 2019
@archmoj
Copy link
Contributor Author

archmoj commented Jul 30, 2019

Peek 2019-07-30 16-56

s.brushStartCallback();
}

var dragging = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Using a module-scope variable will lead to bugs on graphs with multiple parcoords traces (once #3361 is fixed 😏 )

Can you stash dragging on d on some other object that gets passed around here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etpinard Good call.
Done in 30876b2.

}

function dragend(lThis, d) {
if(!dragging) { // i.e. click
Copy link
Contributor

Choose a reason for hiding this comment

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

Found it! Is this the only "new" piece of logic?


Moving the mousemove, drag start/end handler out of attachDragBehavior made this diff was a little hard to read. For next time. having 1 commit that splits attachDragBehavior and then 1 commit that fixes the bug would have been much appreciated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's the only new logic.

Copy link
Contributor

@etpinard etpinard Jul 31, 2019

Choose a reason for hiding this comment

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

... to add to my "for next time" comment, if splitting work into two commits ends up being too tedious, you can alternatively leave it as one commit and highlight the "new" logic using a Github comment on the PR.

mouseClick(295, 200);
delay(snapDelay)()
.then(function() {
expect(gd._fullData[0].dimensions[1].constraintrange).toBeCloseToArray([[0.75, 2.25], [2.75, 3.25]]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice test.

Before this patch gd._fullData[0].dimensions[1].constraintrange evaluated to [2.75, 3.25]

@etpinard
Copy link
Contributor

💃 💃

@archmoj archmoj merged commit c80eb5a into master Jul 31, 2019
@archmoj archmoj deleted the phx235-dont-drop-constraint-onclick branch July 31, 2019 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants