Single API for animating colors and other properties #6191
Conversation
|
Hi @yassipad thanks for the PR. Please remove the built files from your build before committing.
|
src/mixins/animation.mixin.js
Outdated
| @@ -174,6 +174,8 @@ fabric.util.object.extend(fabric.Object.prototype, /** @lends fabric.Object.prot | |||
| propPair = property.split('.'); | |||
| } | |||
|
|
|||
| var propIsText = _this.colorProperties.includes(property) || (propPair && _this.colorProperties.includes(propPair[propPair.length - 1])); | |||
There was a problem hiding this comment.
we still support full es5 browsers, and includes is not part of it, you have to use indexOf.
Also propPair is supported only as a 2 word key, like shadow.color please use propPair[1] instead of length - 1
There was a problem hiding this comment.
sure thing, though actually this is also another issue i've faed when updating points-based shapes (e.g. Polyline) as it requires at least a three-level props tree. Would it be something you'd consider useful to allow for more than just two-level? i made a fix for my project that works well, so i can always make another PR
src/mixins/animation.mixin.js
Outdated
| @@ -174,6 +174,8 @@ fabric.util.object.extend(fabric.Object.prototype, /** @lends fabric.Object.prot | |||
| propPair = property.split('.'); | |||
| } | |||
|
|
|||
| var propIsText = _this.colorProperties.includes(property) || (propPair && _this.colorProperties.includes(propPair[propPair.length - 1])); | |||
There was a problem hiding this comment.
propIsText can be called propIsColor and can be reused later.
src/mixins/animation.mixin.js
Outdated
| }); | ||
| }; | ||
|
|
||
| if (_this.colorProperties.includes(property) || propPair && _this.colorProperties.includes(propPair[propPair.length - 1])) { |
There was a problem hiding this comment.
this is the same check as above, and we can reuse the same boolean we already calculated.
There was a problem hiding this comment.
you're right, i'll change that
| */ | ||
| colorProperties: ( | ||
| 'fill stroke backgroundColor borderColor' | ||
| ).split(' '), |
There was a problem hiding this comment.
is borderColor something we should consider animatable? if yes, why not also cornerColor, cornerStrokeColor and all the rest?
I would leave borderColor out for now. But of course if you think is really important, argument why.
There was a problem hiding this comment.
i personally wouldn't use either, it was just for testing so I think we can leave it out for now
| if (index === properties.length - 1) { | ||
| done(); | ||
| } | ||
| }, 1000); |
There was a problem hiding this comment.
can we use the animation callback instead of a timeout?
There was a problem hiding this comment.
sure, i was just copying the other tests but i can do that! you mean in a onComplete callback?
|
Hey, thanks for your quick feedback, i'll do the necessary changes :) |
| */ | ||
| colorProperties: ( | ||
| 'fill stroke backgroundColor' | ||
| ).split(' '), |
There was a problem hiding this comment.
i want to merge this as it is, but i think i will come back and move colorProperties to animation util level maybe.
Too busy to think of it now and not so important.
There was a problem hiding this comment.
I see, the reason I put it on the object class is because I thought it might be easier to create a new shape class with a custom property name that involved coloring but as you like :)
|
thanks! |
Currently, I cannot animate colors using the animate method of my objects as I would do for other properties. This PR allows fabric to check whether the current animated property should be considered a "colorProperty" and call the animating method accordingly. It adds the "colorProperties" property to the fabric.Object prototype to allow for extension.
Tests are included.
Here is a working demo: https://jsfiddle.net/pzb6yno5/