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

Nested <svg> removal #415

Merged
merged 7 commits into from
Apr 13, 2016
Merged

Nested <svg> removal #415

merged 7 commits into from
Apr 13, 2016

Conversation

mdtusz
Copy link
Contributor

@mdtusz mdtusz commented Apr 11, 2016

@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

  • Swap subplot and annotation <svg> tags for <g> tags
  • Add clip path for traces on the plot area
  • Replace viewBox positioning on drag/movement transform: translate(dx, dy)
  • Test Snapshot.toSVG output for multiple <svg> tags (naively, but it checks what needs to be checked).

Remaining

  • Geo plots still use <svg> tags and will need to be worked on to swap them over to <g> tags.
  • Perhaps a test that will run on all of our mocks to check for nested <svg>?

This change is Reviewable

@etpinard
Copy link
Contributor

@mdtusz Note that geo subplots are exported within the main <svg> via tosvg.js. So they should work fine in Illustrator, etc.

@etpinard
Copy link
Contributor

@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)');
Copy link
Contributor

Choose a reason for hiding this comment

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

is this necessary?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

@etpinard
Copy link
Contributor

pinging @alexcjohnson - this is happening!

}).then(function(svg) {
var splitSVG = svg.split('<svg');

expect(splitSVG.length).toBe(2);
Copy link
Contributor

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(),
Copy link
Contributor

Choose a reason for hiding this comment

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

🍻

@timelyportfolio
Copy link
Contributor

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

image

image

@cpsievert, do you have any other known failures that we could test against?

@etpinard
Copy link
Contributor

@timelyportfolio thanks! That's all I wanted to test for now!

@cpsievert
Copy link

@timelyportfolio nothing in particular, though it wouldn't hurt to quickly test out WebGL as well

@etpinard
Copy link
Contributor

💃 after 1.9.0

@mdtusz mdtusz merged commit 21f5cc9 into master Apr 13, 2016
@mdtusz mdtusz deleted the svg-removal branch April 13, 2016 14:13
@mdtusz mdtusz restored the svg-removal branch April 13, 2016 15:16
@mdtusz mdtusz mentioned this pull request Apr 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants