Skip to content

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

Closed
wants to merge 7 commits into from

Conversation

n-riesco
Copy link
Contributor

@n-riesco n-riesco commented Apr 5, 2016

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?

n-riesco added 3 commits April 5, 2016 08:46
* Removed call to `.remove()` in `shapes.drawAll()`, since `.remove()`
  is also call in `shapes.draw()`.

* No new test failed after this change.
@mdtusz mdtusz changed the title Fix #359 Fix zoom overlay positioning (Fixes #359) Apr 5, 2016
// so they bind to the right events
gd._fullLayout._shapelayer
.selectAll('[data-index="' + (i+1) + '"]')
.attr('data-index', String(i));
Copy link
Contributor

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.

@mdtusz
Copy link
Contributor

mdtusz commented Apr 5, 2016

Looks good so far. For testing, in test/jasmine/assets/mouse_event.js you'll find a method for "faking" mouse events. One test could be to simulate a click&drag zoom using that, and verify that the zoom overlay and shapes are drawn in the correct layers.

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

@n-riesco
Copy link
Contributor Author

n-riesco commented Apr 5, 2016

@mdtusz Thanks for the feedback!

I will add a test to select_test.js and create zoom_test.js.

n-riesco added 2 commits April 5, 2016 20:58
* Test whether select elements are appended to the shape layer.

* Test whether lasso elements are appended to the shape layer.
@etpinard
Copy link
Contributor

etpinard commented Apr 6, 2016

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 <svg> nodes. Looks like it does solves issue #359, so @n-riesco 🍻 for that.

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:

Wait, the zoomlayer is in the shape layer? Why??

Commits: df69584 and e342a9e are valuable additions though.

@n-riesco
Copy link
Contributor Author

n-riesco commented Apr 6, 2016

@etpinard I've updated #378 with e342a9e , or do you think it's better I submit a new PR?

@alexcjohnson
Copy link
Collaborator

@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.

@n-riesco
Copy link
Contributor Author

n-riesco commented Apr 6, 2016

@alexcjohnson OK. I guess that once @mdtusz is done with the un-nesting, I can rebase this PR and create a new zoom layer.

@etpinard
Copy link
Contributor

@n-riesco does #439 make this PR obsolete?

@n-riesco
Copy link
Contributor Author

@etpinard No, it doesn't. I will rebase this PR, once #439 is merged. I don't expect the rebase to be difficult.

@n-riesco
Copy link
Contributor Author

Moved to #448

@n-riesco n-riesco closed this Apr 19, 2016
@n-riesco n-riesco deleted the issue-359-zoomlayer branch April 26, 2016 18:28
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.

Zoom overlay drawn beneath shapes.
4 participants