Add support for non-scaling-stroke vector effect.

[alexgurr]

Hey!

I've added new scaling support for path lengths and included a test with two different kinds of path (identical other than this attribute).

Fixes #112

Would love someone to test a bunch of SVGs manually to prove this, but I've tested with my own line graph and it works.

[maxwellito]

Thanks @alexgurr for your PR! The code seems great, I need to test it as well with few examples tho. Otherwise excellent PR, you've updated the tests as well. I try to get back to you once I have few examples. (my bank holiday weekend is a bit chaotic but I'll try to be fast)

[alexgurr]

Thanks for the kind words. No rush, I'm currently using a GitHub path in my package json file! Test it thoroughly!

I thought about a manual svg test but seemed to be overly complex (the test index.html file didn't even render any SVGs).

[maxwellito]

Ok, after few tests I remember why I didn't implemented this back in the days. It's very tough (if not impossible) to provide an correct value of the path length after transformation. Because the calculation will be based on the scale (width and height) of the element.

For example: (the diagonal of a square) * 2 !== (diagonal of a square) with scale(2, 1)

I did a little test case : http://maxwellito.github.io/drafts/vivus-issue-234/

The robust solution would be to calculate every path length manually and not via .getTotalLength(). But that's not something I'm ready to jump into!
I'm still digging into maths to check if I'm not missing something that could solve the case. I keep you posted :)

[alexgurr]

I would suggest that a solution that partially works (seems to fix 80% of your examples) is potentially better than nothing?

It also looks to me that the scale calculation works for landscape but not portrait. I wonder if there's a third condition/calculation we need for working out scale in the use case height is larger than width?

[maxwellito]

Definitely, I would make it clear in the docs that the feature is not 100% reliable but should fit most cases.

I was investigating to use width and height (cf draft) but still not perfect. I was also looking is the Pythagorean theorem, but still looking :)

Also, this make me think that, when Vivus is parsing a SVG with non-scaling-stroke, it must be remapped when the SVG is resized (due to window resize of other event).

[alexgurr]

Also, this make me think that, when Vivus is parsing a SVG with non-scaling-stroke, it must be remapped when the SVG is resized (due to window resize of other event). - I noticed this. In the app I'm using this library, I just kill the animation and reset the stroke array with !important in the parent. Wasn't sure if we wanted to handle this in the library. I would suggest if we do, we might need to attach an optional observer to the SVG for when it changes size and recalculate the stroke as you mentioned.

I can try and help with some further stuff here if you can provide me that debug HTML and how to run it? My biggest issue was I couldn't run any of the manual examples.

[maxwellito]

Hey @alexgurr

So, after a lot of tests cases I reached this conclusion: while scaling for a line length, it's fine to go bigger than smaller. It might make the line appearing super fast in the worst case scenario, but won't start/stop right in the middle (and look terribly clunky). To do so, I take the biggest scale between X-axis and Y-axis, because it will cover the extreme cases.

I updated my draft : diff and demo

About the re-mapping, I'm happy to ask users to make a call to this.mapping() + this.drawer() (but I need some proper testing on this tho)

Tell me what you think :)

[alexgurr]

Yeah looks good to me. I think that's a reasonable compromise for a consistently working solution.

I think we can should add a section in the documentation about non-scaling-stroke and any caveats. We can then add a sub-section about resizing, maybe explaining the use of mapping, draw and reset and an observer, maybe with a code example? There are multiple ways people could handle resizing. Again, as I mentioned above, I simply override the inline stroke styles with 0 and !important. As you prefer though.

Feel free to close this PR and merge your PR/code in (the automated test is probably still relevant, although the value might need tweaking based on your new scale calculation. Probably also need a few tests to cover different SVG aspect ratios). I like your demo - I'd like to see it added to the repo as well. Nice work :)

(On a side note, drawer is a mis-spell [means something different] and should simply be draw. Not sure how you'd change it now and stay backwards compatible. Maybe rename it, then add a prototype.drawer = this.draw and a comment it's there for backwards compatibility?).

[maxwellito]

Hehe, I know how bad this codebase is about spelling mistakes. My english has slightly improved since. But I never thought this little library would get so much attention. I'll make some changes, feel free to add more :)

About the inline style, I'd like to leave it to .mapping. Using inline style is already a trick to get the highest priority, I don't want to make it more complex with !important tags.

I have only one concern, its about leaving the responsibility to the user to set up an observer when the SVG has non-scaling-vector props. The most common issue I had to face with this project was users not understanding the difference between a stroke based SVG, and a filled one. This is why I doubt a lot about users understanding the implications of the non-scaling. I have 2 approaches:

• Vivus set up a Resize Observer on the SVG and update the path when needed
• Ask users to set up the observer themselves. But show a warning when a non-scaling-vector is detected.

About your PR, let's keep it, if you don't mind I would like to make PRs to your branch (or code suggestions) so we can keep the conversation here. I'll add examples in the manual tests as well.

[alexgurr]

The !important styles are just hacks in my app to get round the animation reset. I'd never expect to implement it in the library.

I see what you're saying. I guess it's as simple as do we take the time to build the observer logic or do we offload that to the user? I think it's also based on what the user of the library wants? For example for me, I'd rather just finish the animation immediately if the window is resized, rather than try and dynamically try and change the stroke offset mid-animation (I think the experience would be better).

My only concern with the observer approach is that it's natively throttled. This means (in theory), as the window is resized there will be slight delays in the calculation and change of stroke offset. This will mean there's always weird gaps (briefly) as the resizing occurs. Observation resizers are also one way of doing this. Potentially we could make some recommendations/examples but let the user decide?

tl;dr I think the warning approach and some good documentation/examples in the README (a 'kill' animation approach and a window observer approach) is the best way to let the user decide how they want to tackle it.

[maxwellito]

Let's do this. I'm starting the implementation now.

Sorry about the delay in my responses, there are big changes in my personal and professional life. It's tough to find some spare time :-D

[maxwellito]

Ok, I have most of it : code change, readme update, new manual test. However, I still have an issue with the Observer. I will have a closer look later

[alexgurr]

Ok this is good to go now. 🎉

[maxwellito]

Ready to go!

@alexgurr thank you very much for your patience and going thru it with me. I wish everybody in the OSS world would take time, as you did, to implement features that matter to them. Big kudos to you

[alexgurr]

Thanks for your time too :). It'll make an appearance in my package react-simple-line-chart when I get round to finishing it!