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

2.5.0 release #595

Closed
10 tasks done
dotnetCarpenter opened this issue Feb 20, 2017 · 28 comments
Closed
10 tasks done

2.5.0 release #595

dotnetCarpenter opened this issue Feb 20, 2017 · 28 comments
Assignees
Milestone

Comments

@dotnetCarpenter
Copy link
Member

dotnetCarpenter commented Feb 20, 2017

Things that needs to be visited before we release 2.5.0

@dotnetCarpenter dotnetCarpenter added this to the 2.5.0 release milestone Feb 20, 2017
@dotnetCarpenter dotnetCarpenter self-assigned this Feb 20, 2017
@Fuzzyma
Copy link
Member

Fuzzyma commented Feb 20, 2017

The relative values for svg number is an fx internal. There is no need to mention that in the docs except if you want to make relative numbers a feature (I do not suggest that ^^)

@wout
Copy link
Member

wout commented Feb 20, 2017

Not sure about the relative value for SVG.Number, have to look into that.

Agree with all but plot() as a getter. The getter persona of the plot() method, to be honest, it feels a bit awkward. Yet another method to get the array while we already have the array() method? That's why I created array() in the first place. If you want the array of an object, do you call plot()? Or do you call array()?

For 3.0.0 this should be revised. In this case, if we keep plot() as the getter, the array() method should be removed altogether. I don't like having multiple methods returning the same value if they do not both serve a specific purpose.

I agree it is in line with the attr() method, though. One ruling them all, that's a good thing. Keeps things simple. It's something to think about going forward. I would like to keep the API lean. Having methods lying around that serve the same purpose as another one, that's a red flag.

The same can be said for the track() method on SVG.TextPath. Maybe it should be called path() (without an argument)? That makes sense...

UPDATE
Maybe plot() should be deprecated in favour of array() in 3.0.0? Just a thought.

@Fuzzyma
Copy link
Member

Fuzzyma commented Feb 20, 2017

This change was done to simplify to initAnimation method of the fx module. There was always one check needed if plot is there so we could get the array with array. It looks much cleaner now. However I agree, that methods with the same functionality are bad code smell. So I agree on your UPDATE

The relative Numbers are exclusively used in the fx module to make dmove, dx... animatable. It is not used anywhere else and isnt a function user would normally need. So I wouldnt mention it in the docs

@wout
Copy link
Member

wout commented Feb 20, 2017

Maybe it's better not to document plot() as getter, for the time being. Just until we decide on the future API.

@wout
Copy link
Member

wout commented Feb 21, 2017

Another thought. SVG.PointArray now accepts a flat array. How about SVG.PathArray?

@Fuzzyma
Copy link
Member

Fuzzyma commented Feb 21, 2017

It already does. See this lines:

      array = array.reduce(function(prev, curr){
        return [].concat.apply(prev, curr)
      }, [])

All arrays are normalized to flat arrays before beeing parsed to a patharray

@wout
Copy link
Member

wout commented Feb 21, 2017

Ok, couldn't find it at first sight. Then I'll document that as well.

@wout
Copy link
Member

wout commented Feb 21, 2017

There is not really a place for this one:

Documentation for allow plot to be called with 4 parameters when animating an SVG.Line

The corresponding methods on animate() should work exactly the same as the non-animated version. So I think we can skip this one.

@RmiTtro
Copy link
Collaborator

RmiTtro commented Feb 21, 2017

Maybe we could mention that plot can be animated in SVG.Line as we do it for SVG.Polyline, SVG.Polygon, etc...

@wout
Copy link
Member

wout commented Feb 21, 2017

Good one, I'll include it the next time I update the docs.

It might be a good idea to mention at method level if they are animateable. Something like:

animateable-better

@RmiTtro
Copy link
Collaborator

RmiTtro commented Feb 21, 2017

I think that would be a good idea and since animated method are made to be used the same way as their non-animated counter-part that might be all that is needed to mention.

@wout
Copy link
Member

wout commented Feb 21, 2017

Great, I'll do that then.

What would be better? Mentioning animateable or the one below?

animate-yes

@RmiTtro
Copy link
Collaborator

RmiTtro commented Feb 21, 2017

Both are fine but I think I will cast my vote on the second one.

@wout
Copy link
Member

wout commented Feb 21, 2017

Me too. Let's go for that then.

@dotnetCarpenter
Copy link
Member Author

@Fuzzyma just a note, but reading the array flatten code:
It should be more efficient if we didn't create an Array instance, just to use its concat method.

array = array.reduce(function(prev, curr){
    return Array.prototype.concat.apply(prev, curr)
}, [])

I don't know how hot this code is. If it's only called a few times, then it makes no difference.

@Fuzzyma
Copy link
Member

Fuzzyma commented Feb 23, 2017

I dont think it makes a big difference but go forward and change it if you like. Actually we can test that with jsperf :D

@dotnetCarpenter
Copy link
Member Author

Si! But you need a throw a lot of arrays after it to see a difference and last time I tried, jsperf broke.

@dotnetCarpenter
Copy link
Member Author

Also I don't think the speed will change much, however I expect the memory usage to be quite different.

@Fuzzyma
Copy link
Member

Fuzzyma commented Feb 23, 2017

In this case I can argue that the array is thrown away in the next run so there is basically only one wasted array in memory every time :)

@dotnetCarpenter
Copy link
Member Author

@Fuzzyma gotcha

@Fuzzyma
Copy link
Member

Fuzzyma commented Feb 23, 2017

Here you go:
https://jsperf.com/concat-vs-array-prototype-concat

Does not make much of a difference. FF is almost as twice as fast as chrome btw^^

@peterennis
Copy link

@dotnetCarpenter I would like to see #597 on the list for this release.

@Fuzzyma
Copy link
Member

Fuzzyma commented Feb 25, 2017

We are all no typescript user. We can put this on the list but only if you are willing to provide us with a pr for this

@peterennis
Copy link

OK. Sounds reasonable. I will look into the ts side as that is what I am using and open tickets as needed. Seems like the 3.0.0 timeline is more realistic.

@Fuzzyma
Copy link
Member

Fuzzyma commented Feb 27, 2017

@peterennis in 3.0 there will be api changes again so the ts file wont be up to date anyway. For user of the v2 branch we maybe should fix it, too. Many users wont switch to v3 immediately

@Fuzzyma
Copy link
Member

Fuzzyma commented Mar 6, 2017

@dotnetCarpenter with #626 all boxes now have a transform function which transforms the box with a given matrix.
I think we can check your last point when merged

@Fuzzyma
Copy link
Member

Fuzzyma commented Mar 10, 2017

issue with jsfiddle created: jsfiddle/jsfiddle-users#983

@Fuzzyma Fuzzyma closed this as completed Mar 10, 2017
@Fuzzyma
Copy link
Member

Fuzzyma commented Mar 10, 2017

They already added it - thats great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants