-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
chore(TS): migrate gradient #8154
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RESULTS on chrome
visual.txt
unit.txt
CI has gone mad saying everything fails
READY on my end
UPDATE
This seemed to be the problem a5c48f5
So useful that error logs point to nothing actionable
Code Coverage Summary
|
Code Coverage Summary
|
NaN || value === value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- renamed
GradientValue
=>GradientCoordValue
- offsetX/Y = 0 class default value
src/gradient/constants.ts
- default coord values- 378ab72 - safegurad
colorStops
from mutation removedBut that is not enough because ofifNaN
- I found out that(NaN || value) === value
0
reverts 684e2c9
Code Coverage Summary
|
// svg goes from internal to external radius. if radius are inverted, swap color stops. | ||
colorStops.reverse(); // mutates array | ||
colorStops.forEach(colorStop => { | ||
colorStop.offset = 1 - colorStop.offset; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would argue this is a mutation too.
As it was in the old code and i wonder how it does not break the code gradient after an svg export.
i will try to write a tests that make this fail, and then fix it.
i think the safest one would be:
colorStops = colorStops.map((colorStop) => ({ ...colorStop, offset: 1 -colorStop.offset }))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it was in the old code and i wonder how it does not break the code gradient after an svg export.
I agree.
i would argue this is a mutation too.
Disagree since colorStops
is a local copy, see assignment with concat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is still mutation because colorStops is a local copy of the array, not of the object contained inside. Those are still references to the original objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh shit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why didn't you commit? OK #8196
this.type = type; | ||
this.gradientUnits = gradientUnits; | ||
this.gradientTransform = gradientTransform || null; | ||
this.offsetX = offsetX; | ||
this.offsetY = offsetY; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok so is there any reason why we want to specify those manually while before we weren't?
Coords and colorStops make sense, but the rest i would rather assign or take the default from class declaration.
constructor({
coords,
colorStops = [],
id,
...restOfOptions,
}: GradientOptions<T>) {
....
Object.assign(this, restOfOptions)
why going for the explicit way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a good reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accept that it keeps junk passed down in options out of the class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is exactly what we do with objects on the otherside.
We allow devs to pass id, name, and other things they may want to use.
For me it needs to be clarified, not blocked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree and TS will disagree as well.
This is the exact use case of a subclass.
Stricter js is part of modern js.
It's how I see it.
I have one outstanding comment, all the rest seems good to me. |
Because the test actually fails, see #8168. Something weird is happening with text and gradients/patterns. Sometimes it renders perfectly and sometimes it renders nothing. |
Code Coverage Summary
|
Code Coverage Summary
|
Co-authored-by: Andrea Bogazzi <andreabogazzi79@gmail.com>
merge chore(TS): prepare for gradient migration #8155update from masterresolve conflictsours
strategy (ping me if you are not sure and request my review to be certain)READY