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.

Sign up for GitHub

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

Jump to bottom

Convert Rect to Es6, remove global scope function. #8118

Merged
merged 14 commits into from
Aug 4, 2022
Merged

Convert Rect to Es6, remove global scope function. #8118

merged 14 commits into from
Aug 4, 2022

Conversation

asturur
Copy link
Member

@asturur asturur commented Aug 4, 2022

No description provided.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022 •
edited
Loading

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.21 |    74.62 |   84.48 |   80.79 |                                               
 fabric.js |   82.21 |    74.62 |   84.48 |   80.79 | ...,27463,27473-27517,27625,27712,27916,28057 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022 •
edited
Loading

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.16 |    74.59 |   84.48 |   80.74 |                                               
 fabric.js |   82.16 |    74.59 |   84.48 |   80.74 | ...,27463,27473-27517,27625,27712,27916,28057 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022 •
edited
Loading

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.16 |    74.59 |   84.48 |   80.74 |                                               
 fabric.js |   82.16 |    74.59 |   84.48 |   80.74 | ...,27463,27473-27517,27625,27712,27916,28057 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022 •
edited
Loading

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.16 |    74.59 |   84.48 |   80.74 |                                               
 fabric.js |   82.16 |    74.59 |   84.48 |   80.74 | ...,27463,27473-27517,27625,27712,27916,28057 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022 •
edited
Loading

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.16 |    74.59 |   84.48 |   80.74 |                                               
 fabric.js |   82.16 |    74.59 |   84.48 |   80.74 | ...,27463,27473-27517,27625,27712,27916,28057 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022 •
edited
Loading

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.15 |    74.55 |   84.48 |   80.74 |                                               
 fabric.js |   82.15 |    74.55 |   84.48 |   80.74 | ...,27470,27480-27524,27632,27719,27923,28064 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022 •
edited
Loading

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.15 |    74.55 |   84.48 |   80.74 |                                               
 fabric.js |   82.15 |    74.55 |   84.48 |   80.74 | ...,27470,27480-27524,27632,27719,27923,28064 
-----------|---------|----------|---------|---------|-----------------------------------------------

@asturur asturur changed the title Find where shape migration breaks Convert Rect to Es6, remove global scope function. Aug 4, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022 •
edited
Loading

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.15 |    74.54 |   84.49 |   80.73 |                                               
 fabric.js |   82.15 |    74.54 |   84.49 |   80.73 | ...,27490,27500-27544,27652,27739,27943,28084 
-----------|---------|----------|---------|---------|-----------------------------------------------

@asturur asturur mentioned this pull request Aug 4, 2022
Closed
@asturur
Copy link
Member Author

asturur commented Aug 4, 2022

@ShaMan123 i re-enabled Lint because this will force people working on a file to clean their file.
When we evaluate a PR let's check that in the main file Lint has been fixed.

@asturur
Copy link
Member Author

asturur commented Aug 4, 2022

image

ShaMan123
ShaMan123 reviewed Aug 4, 2022
View reviewed changes
Copy link
Contributor

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Rect!

src/shapes/rect.class.ts Outdated
const x = -w / 2;
const y = -h / 2;
rx = rx ? Math.min(rx, w / 2) : 0;
ry = this.ry ? Math.min(ry, h / 2) : 0;
Copy link
Contributor

@ShaMan123 ShaMan123 Aug 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can inline this and make a const

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you mean?

src/shapes/rect.class.ts
* @memberOf Rect
* @see: http://www.w3.org/TR/SVG/shapes.html#RectElement
*/
Rect.ATTRIBUTE_NAMES = fabric.SHARED_ATTRIBUTES.concat('x y rx ry width height'.split(' '));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like the 'a b c'.split(' ') concept
Can we dump it?
TS won't like it either if we enforce types on ATTRIBUTE_NAMES

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can dump it yes, we will probably have enums for those, but right now i really don't care.

src/shapes/rect.class.ts
return callback(null);
}
const {
left = 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WOW I didn't know this was possible!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this highligtherd an issue of this code.
visible needs to be default true. Because ti requires the knowledge of the fact that the attribute default value on SVG is true. It broke my tests for long, i tought it was an es6 issue, while it was just a real code change i introduced without knowing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

visible false is hidden, visible undefined means visible.

src/shapes/rect.class.ts Outdated
visible = true,
...RestOfparsedAttributes
} = fabric.parseAttributes(element, Rect.ATTRIBUTE_NAMES);
const rect = new Rect(Object.assign({}, options, RestOfparsedAttributes, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assign go away!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RestOfparsedAttributes -> restOfParsedAttributes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh right let's remove this.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022 •
edited
Loading

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.15 |    74.54 |   84.49 |   80.73 |                                               
 fabric.js |   82.15 |    74.54 |   84.49 |   80.73 | ...,27488,27498-27542,27650,27737,27941,28082 
-----------|---------|----------|---------|---------|-----------------------------------------------

ShaMan123
ShaMan123 reviewed Aug 4, 2022
View reviewed changes
src/shapes/rect.class.ts Outdated
@@ -77,7 +77,7 @@ const Rect = fabric.util.createClass(fabric.Object, /** @lends Rect.prototype */
const x = -w / 2;
const y = -h / 2;
rx = rx ? Math.min(rx, w / 2) : 0;
ry = this.ry ? Math.min(ry, h / 2) : 0;
ry = ry ? Math.min(ry, h / 2) : 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant

Suggested change
ry = ry ? Math.min(ry, h / 2) : 0;
const ry = this.ry ? Math.min(this.ry, h / 2) : 0;

and remove let ry

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok now i done it differently. is not really an issue.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022 •
edited
Loading

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.15 |    74.54 |   84.49 |   80.73 |                                               
 fabric.js |   82.15 |    74.54 |   84.49 |   80.73 | ...,27487,27497-27541,27649,27736,27940,28081 
-----------|---------|----------|---------|---------|-----------------------------------------------

@asturur asturur merged commit 60a92a7 into master Aug 4, 2022
@asturur asturur deleted the rect-take-3 branch September 11, 2022 23:03
frankrousseau pushed a commit to cgwire/fabric.js that referenced this pull request Jan 6, 2023
@asturur @frankrousseau
Chore(TS) Rect to Es6, remove global scope function. (fabricjs#8118)
d3ec63a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShaMan123 ShaMan123 ShaMan123 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@asturur @ShaMan123