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

chore(TS): Color #8115

Merged
merged 6 commits into from Aug 4, 2022
Merged

chore(TS): Color #8115

merged 6 commits into from Aug 4, 2022

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Aug 4, 2022

Color class

I have created a patch index for util so we can start importing from there

I don't like that the history isn't kept between src/color.class.ts -> src/color/color.class.ts so I think we should open a PR that only moves the file and then merge this one and do that in future PRs

EDITED
Merge #8116 BEFORE this PR, update from master and merge this

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022

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.57 |   84.49 |   80.72 |                                               
 fabric.js |   82.15 |    74.57 |   84.49 |   80.72 | ...,27471,27481-27525,27633,27720,27924,28065 
-----------|---------|----------|---------|---------|-----------------------------------------------

@ShaMan123 ShaMan123 requested a review from asturur August 4, 2022 03:36
@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022

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.57 |   84.49 |   80.72 |                                               
 fabric.js |   82.15 |    74.57 |   84.49 |   80.72 | ...,27464,27474-27518,27626,27713,27917,28058 
-----------|---------|----------|---------|---------|-----------------------------------------------

composeMatrix,
rotatePoint
};
export default fabric.util;
Copy link
Member

Choose a reason for hiding this comment

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

let's not use default exports, is a mess when we mix default and named exports.
Let's merge this PR. But in general this kind of extraction i think is actual code more than a bunch of import/exports that goes away at rollup time. I would like to use named exports only unless we have some kind of concerns internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure
I want named exports as well.
Just for hacking the build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well maybe we should remove this line
nothing should use it

asturur
asturur previously approved these changes Aug 4, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022

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.57 |   84.49 |   80.74 |                                               
 fabric.js |   82.16 |    74.57 |   84.49 |   80.74 | ...,27465,27475-27519,27627,27714,27918,28059 
-----------|---------|----------|---------|---------|-----------------------------------------------

@asturur asturur merged commit 08229cf into master Aug 4, 2022
frankrousseau pushed a commit to cgwire/fabric.js that referenced this pull request Jan 6, 2023
Co-authored-by: Andrea Bogazzi <andreabogazzi79@gmail.com>
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

Successfully merging this pull request may close these issues.

None yet

2 participants