fix ternary hover after zoom/pan events #688
Conversation
registered after zooming. This fixes that, and tests the fix.
select_test had a slight variant with the callback built into doubleClick - changed it into the other form where you just do .then(callback)
Dunno why eslint cannot find the config file correctly from the test directory when I'm using sublime...
|
I consolidated all the separate definitions of |
| }; |
There was a problem hiding this comment.
@etpinard per my commit message... somehow eslint run through sublime cannot sort out the config files within the jasmine directory, even though as far as I can tell it's all correct. In any other directory things look fine. Have you seen this? If I change test/jasmine/.eslintrc from "extends": "../../.eslintrc" to "extends": "../.eslintrc" it no longer complains, though that's clearly not what you want as it'll think node is allowed.
I suppose we could make that change and also explicitly set env.node=false, that seems to work for me, but it's strange that this would be necessary.
There was a problem hiding this comment.
Interesting. No, I've never seen that one before.
There was a problem hiding this comment.
I was thinking it may be worth moving all our .eslintrc in a config/ at the root of the repo instead of having them scattered around (and hidden in) the directory tree.
We could move test_syntax.js to tasks/.
Having
test/
|
- image/
- jasmine/
- test_syntax.js
- .eslintrc (which is only relevant for test_syntax.js)
isn't great.
| // constrain ymin/max to the visible plot, so the label goes | ||
| // at the middle of the piece you can see | ||
| ymin = Math.max(ymin, 0); | ||
| ymax = Math.min(ymax, ya._length); |
There was a problem hiding this comment.
So if you have, for example, a shape that goes way off the top of the plot, the label will still be centered on the visible portion rather than pinned to the top (and because of how this happens, the x position of the label could also have ended up screwy previously)
There was a problem hiding this comment.
Great. That sounds good.
Now that all the scatter ternary hover label testing infrastructure is in place, would you mind adding a test case for this?
|
This latest commit about label positions addresses a different issue from the ternary hover disappearing entirely... but I added it here because a) I needed to fix that before I could even see the zoomed-in label positions, and b) it still fits the PR title "fix ternary (and scatter!) hover after zoom/pan events" |
| // from the mousemove events and then simulate | ||
| // a click event on mouseup | ||
| var click = require('../assets/click'); | ||
| var doubleClick = require('../assets/double_click'); |
There was a problem hiding this comment.
Fantastic. Thanks so much!
|
Thanks for the hot fix! 💃 after #688 (comment) is done. |
#660 had a weird interaction with ternary that hover events stopped being registered after zooming. This fixes that, and tests the fix.
@n-riesco @etpinard As I mentioned in slack, probably longer-term these mousemove/mouseout/click event handlers should get subsumed into dragElement... but this fixes the issue for now.