chore(TS): migrate gradient #8154

ShaMan123 · 2022-08-18T08:50:23Z

~~merge chore(TS): prepare for gradient migration #8155~~
~~update from master~~
~~resolve conflicts ours strategy (ping me if you are not sure and request my review to be certain)~~

READY

ShaMan123

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

src/gradient/gradient.class.ts

src/parser/percent.ts

github-actions · 2022-08-24T05:22:54Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.04 |    74.65 |   84.52 |   80.64 |                                               
 fabric.js |   82.04 |    74.65 |   84.52 |   80.64 | ...,27409,27467,27477-27521,27629,27716,27920 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-08-24T05:33:46Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.03 |    74.62 |   84.52 |   80.63 |                                               
 fabric.js |   82.03 |    74.62 |   84.52 |   80.63 | ...,27409,27467,27477-27521,27629,27716,27920 
-----------|---------|----------|---------|---------|-----------------------------------------------

NaN || value === value

ShaMan123

renamed GradientValue => GradientCoordValue
offsetX/Y = 0 class default value
src/gradient/constants.ts - default coord values
378ab72 - safegurad colorStops from mutation
~~removed ifNaN - I found out that (NaN || value) === value~~ But that is not enough because of 0

reverts 684e2c9

github-actions · 2022-08-24T06:38:03Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.04 |    74.65 |   84.52 |   80.64 |                                               
 fabric.js |   82.04 |    74.65 |   84.52 |   80.64 | ...,27409,27467,27477-27521,27629,27716,27920 
-----------|---------|----------|---------|---------|-----------------------------------------------

closes #8168

asturur · 2022-08-24T21:39:03Z

src/gradient/gradient.class.ts

+        // 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;


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 }))

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

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.

~~Why didn't you commit?~~ OK #8196

asturur · 2022-08-24T21:48:22Z

src/gradient/gradient.class.ts

+    this.type = type;
+    this.gradientUnits = gradientUnits;
+    this.gradientTransform = gradientTransform || null;
+    this.offsetX = offsetX;
+    this.offsetY = offsetY;


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?

~~I don't have a good reason~~

~~accept that~~ it keeps junk passed down in options out of the class

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

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.

asturur · 2022-08-24T21:50:31Z

I have one outstanding comment, all the rest seems good to me.
The more restrictive percentage check for text and gradients does not seem to work well when running coverage

ShaMan123 · 2022-08-25T05:13:56Z

I have one outstanding comment, all the rest seems good to me. The more restrictive percentage check for text and gradients does not seem to work well when running coverage

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.

github-actions · 2022-08-26T22:31:48Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.05 |    74.69 |   84.43 |   80.67 |                                               
 fabric.js |   82.05 |    74.69 |   84.43 |   80.67 | ...-27584,27700-27701,27722-27763,27778-27937 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-08-26T22:52:27Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.04 |    74.66 |   84.43 |   80.66 |                                               
 fabric.js |   82.04 |    74.66 |   84.43 |   80.66 | ...-27584,27700-27701,27722-27763,27778-27937 
-----------|---------|----------|---------|---------|-----------------------------------------------

Co-authored-by: Andrea Bogazzi <andreabogazzi79@gmail.com>

ShaMan123 added 14 commits August 18, 2022 08:32

init

58d50a5

major

91f4a89

rename

1be0a07

types

295fad8

Update index.js

29a7ddf

opts

a5f906a

Update typedefs.ts

0ed975b

Update gradient.class.ts

d7c157a

Update gradient.class.ts

b9c62e8

**BREAKING** change fromElement signature

dcfda21

cleanup

4a2cffb

move

f5596a2

Create index.ts

cd6f610

Update index.js

44dfca8

ShaMan123 mentioned this pull request Aug 18, 2022

chore(TS): prepare for gradient migration #8155

Merged

ShaMan123 added 12 commits August 18, 2022 12:53

Update gradient.class.ts

df8d16b

Update gradient.class.ts

e4b257d

fix types

3ae0d3a

Update gradient.class.ts

60fbc6e

Update parseCoords.ts

3ce2adb

ifNaN

e219b40

rename

c7a40ee

safeguard from null

2bc3dd7

clamp valueIfNaN

582d1cc

Update percent.ts

d81ef06

Update percent.ts

cf6e340

Update ifNaN.ts

8a11582

ShaMan123 commented Aug 18, 2022

View reviewed changes

Merge branch 'ts-gradient-pre' into ts-gradient2

b07fa0d

ShaMan123 requested a review from asturur August 18, 2022 16:21

Update index.ts

a5c48f5

Update gradient.class.ts

378ab72

ShaMan123 added 2 commits August 24, 2022 09:10

remove ifNaN

684e2c9

NaN || value === value

checkout

04634e1

ShaMan123 commented Aug 24, 2022

View reviewed changes

revert remove ifNaN

fa38d1d

reverts 684e2c9

ShaMan123 mentioned this pull request Aug 24, 2022

Text fill render issue when gradient unit is percentage #8168

Open

1 task

fix visual test percision

7f68645

closes #8168

asturur reviewed Aug 24, 2022

View reviewed changes

ShaMan123 and others added 8 commits August 25, 2022 12:16

Merge branch 'master' into ts-gradient2

6625eb1

Merge branch 'master' into ts-gradient2

ee14fda

Merge branch 'master' into ts-gradient2

0cbef4a

fix(): naming error

2078c9c

Merge branch 'master' into ts-gradient2

1ee19ec

Merge branch 'master' into ts-gradient2

329e23c

test more strict

1c38116

restore text9.png

63c4f91

remove kerning to lessen differences between oses

df2acd4

asturur approved these changes Aug 26, 2022

View reviewed changes

asturur merged commit 16f5bcb into master Aug 26, 2022

asturur deleted the ts-gradient2 branch September 11, 2022 23:03

frankrousseau pushed a commit to cgwire/fabric.js that referenced this pull request Jan 6, 2023

chore(TS): migrate gradient (fabricjs#8154)

4506b03

Co-authored-by: Andrea Bogazzi <andreabogazzi79@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(TS): migrate gradient #8154

chore(TS): migrate gradient #8154

ShaMan123 commented Aug 18, 2022 •

edited

ShaMan123 left a comment •

edited

github-actions bot commented Aug 24, 2022 •

edited

github-actions bot commented Aug 24, 2022 •

edited

ShaMan123 left a comment •

edited

github-actions bot commented Aug 24, 2022 •

edited

asturur Aug 24, 2022

ShaMan123 Aug 25, 2022 •

edited

asturur Aug 26, 2022

ShaMan123 Aug 26, 2022

ShaMan123 Aug 26, 2022 •

edited

asturur Aug 24, 2022

ShaMan123 Aug 25, 2022 •

edited

ShaMan123 Aug 25, 2022 •

edited

asturur Aug 26, 2022

ShaMan123 Aug 26, 2022

asturur commented Aug 24, 2022

ShaMan123 commented Aug 25, 2022

github-actions bot commented Aug 26, 2022 •

edited

github-actions bot commented Aug 26, 2022 •

edited

chore(TS): migrate gradient #8154

chore(TS): migrate gradient #8154

Conversation

ShaMan123 commented Aug 18, 2022 • edited

ShaMan123 left a comment • edited

Choose a reason for hiding this comment

github-actions bot commented Aug 24, 2022 • edited

Code Coverage Summary

github-actions bot commented Aug 24, 2022 • edited

Code Coverage Summary

ShaMan123 left a comment • edited

Choose a reason for hiding this comment

github-actions bot commented Aug 24, 2022 • edited

Code Coverage Summary

asturur Aug 24, 2022

Choose a reason for hiding this comment

ShaMan123 Aug 25, 2022 • edited

Choose a reason for hiding this comment

asturur Aug 26, 2022

Choose a reason for hiding this comment

ShaMan123 Aug 26, 2022

Choose a reason for hiding this comment

ShaMan123 Aug 26, 2022 • edited

Choose a reason for hiding this comment

asturur Aug 24, 2022

Choose a reason for hiding this comment

ShaMan123 Aug 25, 2022 • edited

Choose a reason for hiding this comment

ShaMan123 Aug 25, 2022 • edited

Choose a reason for hiding this comment

asturur Aug 26, 2022

Choose a reason for hiding this comment

ShaMan123 Aug 26, 2022

Choose a reason for hiding this comment

asturur commented Aug 24, 2022

ShaMan123 commented Aug 25, 2022

github-actions bot commented Aug 26, 2022 • edited

Code Coverage Summary

github-actions bot commented Aug 26, 2022 • edited

Code Coverage Summary

ShaMan123 commented Aug 18, 2022 •

edited

ShaMan123 left a comment •

edited

github-actions bot commented Aug 24, 2022 •

edited

github-actions bot commented Aug 24, 2022 •

edited

ShaMan123 left a comment •

edited

github-actions bot commented Aug 24, 2022 •

edited

ShaMan123 Aug 25, 2022 •

edited

ShaMan123 Aug 26, 2022 •

edited

ShaMan123 Aug 25, 2022 •

edited

ShaMan123 Aug 25, 2022 •

edited

github-actions bot commented Aug 26, 2022 •

edited

github-actions bot commented Aug 26, 2022 •

edited