-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Nested <svg> removal #415
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
Nested <svg> removal #415
Conversation
@timelyportfolio could you try to pull down this branch and if it indeed fixes #322 ? |
var ann = anng.append('svg') | ||
.call(Plotly.Drawing.setPosition, 0, 0); | ||
var ann = anng.append('g') | ||
.attr('transform', 'translate(0,0)'); |
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.
is this necessary?
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.
Yep - the call to setPosition
only sets x
and y
attributes - for <g>
's, we need to position using transform
.
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.
I suppose we could add a drawing.setTranslate
method though too. Thoughts?
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.
Oh. I meant, 'transform', 'translate(0,0)'
as no effects, right?
By the why, I like the idea of having a drawing.setTranslate
method. No need to do that right now though.
pinging @alexcjohnson - this is happening! |
}).then(function(svg) { | ||
var splitSVG = svg.split('<svg'); | ||
|
||
expect(splitSVG.length).toBe(2); |
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.
Could we make this test even better by searching for one and only one <svg>
and one and only one </svg>
?
@@ -150,7 +150,8 @@ describe('Plotly.Snapshot', function() { | |||
}); | |||
|
|||
describe('toSVG', function() { | |||
var gd; | |||
var parser = new DOMParser(), |
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.
🍻
@etpinard, I did not have time to do a full round of tests, but the results on the known previous failures are very encouraging. In those two cases, they both now correctly snapshot in RStudio Viewer. @cpsievert, do you have any other known failures that we could test against? |
@timelyportfolio thanks! That's all I wanted to test for now! |
@timelyportfolio nothing in particular, though it wouldn't hurt to quickly test out WebGL as well |
💃 after |
@jackparmer @etpinard
After this PR is merged, we should be able to use exported SVG from most plots in vector applications like Illustrator without nesting issues.
In Brief
<svg>
tags for<g>
tagsviewBox
positioning on drag/movementtransform: translate(dx, dy)
Snapshot.toSVG
output for multiple<svg>
tags (naively, but it checks what needs to be checked).Remaining
<svg>
tags and will need to be worked on to swap them over to<g>
tags.<svg>
?This change is