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

Layer fix #2121

Merged
merged 5 commits into from
Oct 27, 2017
Merged

Layer fix #2121

merged 5 commits into from
Oct 27, 2017

Conversation

alexcjohnson
Copy link
Collaborator

fixes #2110 and #2119

  • put layer-above at the bottom of toppaper so shapes (and images) in this layer end up above all trace types
  • put updatemenus into their own layer - menulayer, naturally, because the real reason they need to be on top is that they contain hidden elements that, whenever and wherever they are shown, need to take precedence over everything else.

@@ -57,7 +57,7 @@ function draw(gd) {
function drawOne(gd, index) {
// remove the existing shape if there is one.
// because indices can change, we need to look in all shape layers
gd._fullLayout._paper
gd._fullLayout._paperdiv
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Really we should just move shapes (and annotations for that matter) to the standard d3 update pattern like images already use (and which did not require this kind of change to adapt to the changed layering)... but that's a bigger project than I had an appetite for right now!

Copy link
Contributor

Choose a reason for hiding this comment

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

but that's a bigger project than I had an appetite for right now!

Fixing two 🪲 in one PR, but hungry for more 🍔

Copy link
Contributor

@etpinard etpinard left a comment

Choose a reason for hiding this comment

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

Looks great. I made one potentially blocking comment (low probability), that I'll paste here:

Can anyone think of a situation where one would want to place an updatemenu below other features?

@@ -54,7 +54,7 @@ module.exports = function draw(gd) {
*/

// draw update menu container
var menus = fullLayout._infolayer
var menus = fullLayout._menulayer
Copy link
Contributor

Choose a reason for hiding this comment

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

Ha I guess that does it. I first thought about splitting the updatemenu buttons from their header <g> i.e. the header would have remained in <g infolayer> and the buttons moved to <g menulayer>.

Can anyone think of a situation where one would want to place an updatemenu below other <g infolayer> features?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't see when you'd want the updatemenu below anything else, but I guess it could be useful to separate <g menulayer> itself into two pieces, so that regardless of the order you draw the menus, when they open up the transient parts will be on top. The only case I can think of that would really need this is if you have two menus that each overlaps the other when opened - one above that opens downward and one below that opens upward for example. If you have a lot of menus that might become necessary at times.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updatemenus were already smart enough to put the dropdown buttons (but not always-visible buttons) on top - exactly what I was suggesting 🎉 🏆 But there was a bug with this that if a new menu was added when other menus already existed, this would get added on top of the dropdown buttons -> fixed in e0a10af

@@ -57,7 +57,7 @@ function draw(gd) {
function drawOne(gd, index) {
// remove the existing shape if there is one.
// because indices can change, we need to look in all shape layers
gd._fullLayout._paper
gd._fullLayout._paperdiv
Copy link
Contributor

Choose a reason for hiding this comment

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

but that's a bigger project than I had an appetite for right now!

Fixing two 🪲 in one PR, but hungry for more 🍔

@@ -97,6 +97,9 @@ module.exports = function draw(gd) {

// remove exiting header, remove dropped buttons and reset margins
if(headerGroups.enter().size()) {
// make sure gButton is on top of all headers
gButton.node().parentNode.appendChild(gButton.node());
Copy link
Contributor

Choose a reason for hiding this comment

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

Nicely done.

@etpinard
Copy link
Contributor

Solid fix 💃

@alexcjohnson alexcjohnson merged commit aa67de2 into master Oct 27, 2017
@alexcjohnson alexcjohnson deleted the layer-fix branch October 27, 2017 14:23
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.

Plotly shape always drawn under the trace when the plot type is pie
2 participants