-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix zoom overlay positioning (Fixes #359) #395
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
Conversation
* Removed call to `.remove()` in `shapes.drawAll()`, since `.remove()` is also call in `shapes.draw()`. * No new test failed after this change.
// so they bind to the right events | ||
gd._fullLayout._shapelayer | ||
.selectAll('[data-index="' + (i+1) + '"]') | ||
.attr('data-index', String(i)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI String(i)
is unnecessary - i
will be coerced to a string by .attr
.
Looks good so far. For testing, in e.g. // initiate the click&drag
mouseEvent('mousemove', startX, startY);
mouseEvent('mousedown', startX, startY);
mouseEvent('mousemove', endX, endY);
// check some things
// mouseup to finish the action
mouseEvent('mouseup', toX, toY);
// maybe test some more stuff |
@mdtusz Thanks for the feedback! I will add a test to |
* Test whether select elements are appended to the shape layer. * Test whether lasso elements are appended to the shape layer.
Moving the select and zoombox elements from the subplot svg to the shapelayer is only a short term solution before @mdtusz un-nests the subplot That said, I don't think we should go forward with this PR - as it is only a temporary solution that may lead to confusion before we un-nests the subplot nodes. I can imagine @alexcjohnson saying:
|
@n-riesco I like the way you've put the zoombox in a root layer and added a transform to it; but lets just make it a totally new root layer. I suspect it wants to go above infolayer too, annotations likely have the same problem as shapes at the moment? I suppose we could put it in hoverlayer, since hover info is deleted as soon as zooming starts... but it'd be clearer and more flexible just to give it its own layer. |
@alexcjohnson OK. I guess that once @mdtusz is done with the un-nesting, I can rebase this PR and create a new zoom layer. |
Moved to #448 |
This PR fixes #359 by moving the select and zoombox elements from the subplot svg to the shapelayer.
@etpinard @mdtusz do you have suggestions for any tests I could add?