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

Rangeslider style on select fix #4022

Merged
merged 6 commits into from
Jul 5, 2019
Merged

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented Jul 5, 2019

@plotly/plotly_js - tweaking a few things in selection / rangeslider land.

fixes #3957

before: https://codepen.io/mbkupfer/pen/zVGwoZ
after: https://codepen.io/etpinard/pen/KjxWzZ (try selecting the bars)

- which is a more centralised DRY place
- use nodeRangePlot3 for range-plot trace, that way
  we don't override node3 when plotting a trace in a range-plot
... for modules that define a _module.styleOnSelect method
- as they're only meant to be used on select!
... during selection mousemove.  To do so,
- make _module.styleOnSelect handle a 3rd argument
  (in practice, a d3 selection of <g.trace> of the main plot
  OR the range plot)
- similarly for _module.style methods that get called during selections
- N.B. has to "fix" one (wrong) finance select assertion
@etpinard etpinard added bug something broken status: reviewable labels Jul 5, 2019
@etpinard
Copy link
Contributor Author

etpinard commented Jul 5, 2019

Pinging @mbkupfer who seems to be very good at noticing rangeslider/selection bugs

Could you try https://codepen.io/etpinard/pen/KjxWzZ - if you get the chance? Thanks very much!

@archmoj
Copy link
Contributor

archmoj commented Jul 5, 2019

Looking good to my eyes.
@etpinard would you like to include this in 1.49.0?

@etpinard
Copy link
Contributor Author

etpinard commented Jul 5, 2019

would you like to include this in 1.49.0?

Yeah, that would be nice.

@mbkupfer
Copy link

mbkupfer commented Jul 5, 2019

Excellent. I tried to test out many different scenarios including click events and multiple series and didn't see any sync problems.
Thanks for the fix @etpinard, can't wait for the upgrade!

@archmoj archmoj added this to the v1.49.0 milestone Jul 5, 2019
@archmoj
Copy link
Contributor

archmoj commented Jul 5, 2019

@mbkupfer your review is much appreciated.

@etpinard thanks for the fix.
💃

@etpinard etpinard merged commit 8d4ab24 into master Jul 5, 2019
@etpinard etpinard deleted the rangeslider-styleOnSelect-fixup branch July 5, 2019 17:53
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.

Rangeslider selection display not updating
3 participants