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

ensure we create only one modebar-container #3678

Merged
merged 2 commits into from
Mar 25, 2019
Merged

ensure we create only one modebar-container #3678

merged 2 commits into from
Mar 25, 2019

Conversation

antoinerg
Copy link
Contributor

@antoinerg antoinerg commented Mar 25, 2019

Closes #3677

  • Test that the final DOM sandwich is the one we want ( hoverlabels > modebar > the rest )

@@ -3802,6 +3802,7 @@ function makePlotFramework(gd) {
.classed('gl-container', true);

fullLayout._paperdiv.selectAll('.main-svg').remove();
fullLayout._paperdiv.selectAll('.modebar-container').remove();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We remove the existing modebar because:

// Make the graph containers
// start fresh each time we get here, so we know the order comes out
// right, rather than enter/exit which can muck up the order
// TODO: sort out all the ordering so we don't have to
// explicitly delete anything

@@ -3802,6 +3802,7 @@ function makePlotFramework(gd) {
.classed('gl-container', true);

fullLayout._paperdiv.selectAll('.main-svg').remove();
fullLayout._paperdiv.selectAll('.modebar-container').remove();
Copy link
Contributor

Choose a reason for hiding this comment

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

.select should suffice here, as there's only one <div.modebar-container> per graph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 567edde

@etpinard etpinard added bug something broken status: reviewable labels Mar 25, 2019
expect(hoverlayer.length).toBe(1);

var compareMask = infolayer[0].compareDocumentPosition(modebar[0]);
expect(compareMask).toBe(Node.DOCUMENT_POSITION_FOLLOWING, '.modebar-container appears after the .infolayer');
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow. I didn't know that thing existed. NICE!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Me neither! Just found out about them today: https://developer.mozilla.org/en-US/docs/Web/API/Node/compareDocumentPosition 😄

@etpinard
Copy link
Contributor

Thanks for the quick fix 💃

@antoinerg antoinerg merged commit f0a6128 into master Mar 25, 2019
@antoinerg antoinerg deleted the fix-3677 branch March 25, 2019 19:21
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.

None yet

2 participants