Fix clipping and device size bugs

[yixuan]

This PR fixes #47 and #50 .

[codecov-io]

Current coverage is 93.76%

Merging #55 into master will increase coverage by +0.83% as of 6837d74

@@            master     #55   diff @@
======================================
  Files            6       6       
  Stmts          382     417    +35
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit            355     391    +36
  Partial          0       0       
+ Missed          27      26     -1

Review entire Coverage Diff as of 6837d74

Powered by Codecov. Updated on successful CI builds.

[hadley]

Could you add a unit test for the clipping too please? Maybe something using the problem in #47? (but re-created with base graphics)

[yixuan]

Yeah, I've thought about it, but I actually don't know what we really want to test. It's hard to examine the effectiveness of the clipping purely from the generated SVG code. :-|

Any insights?

[hadley]

Another way to attack this is a regression test: we just save the svg output in the tests directory, so we can at least see if things change. That's particularly nice in combination with github's image diffs, but unfortunately you have to push to see those.

[yixuan]

I agree. I created a "fake" test that will create a plot to highlight the effect of clipping. We can actually see the difference as the plot below shows:

[hadley]

Can you please check that svg file in too?

[yixuan]

Do you mean adding and committing the test-clip.svg file to the test folder?

[hadley]

@yixuan yup

[hadley]

Can you double check that clipPath needs to go in defs? I think that's only necessary if you want to embed at the top of the document, but I might be wrong.

[yixuan]

It is not necessary, but recommended. I quote the following paragraph from the SVG standard:

It is recommended that, wherever possible, referenced elements be defined inside of a ‘defs’ element. Among the elements that are always referenced: ‘altGlyphDef’, ‘clipPath’, ‘cursor’, ‘filter’, ‘linearGradient’, ‘marker’, ‘mask’, ‘pattern’, ‘radialGradient’ and ‘symbol’. Defining these elements inside of a ‘defs’ element promotes understandability of the SVG content and thus promotes accessibility.

[hadley]

Ok, great - thanks!

[hadley]

For future reference, when a pull request closes issues, can you please include in the commit messages so that they get closed automatically on merge? (i.e. include Fixes #47 in one of the commits)

[hadley]

Oh I guess you did, but missed #50

[yixuan]

Sorry I didn't. :-\ It seems that I'm unaware of all these fancy tricks on Github.