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

refactor(object_interactivity): draw borders #7932

Merged
merged 8 commits into from
Jun 16, 2022
Merged

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented May 10, 2022

This PR refactors logic to enable easier overriding + maintenance.

  • exposes private _drawBorders
  • exposes public strokeBorders for easier overriding

BREAKING d257c99:

  • removed drawBordersInGroup
  • changed drawBroders signature

@github-actions
Copy link
Contributor

github-actions bot commented May 10, 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  |   83.03 |    75.97 |    85.7 |   82.75 |                                               
 fabric.js |   83.03 |    75.97 |    85.7 |   82.75 | ...,30832,30906,30917-30982,31105,31204,31440 
-----------|---------|----------|---------|---------|-----------------------------------------------

@ShaMan123 ShaMan123 requested a review from asturur May 10, 2022 11:08
@github-actions
Copy link
Contributor

github-actions bot commented May 10, 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  |   83.02 |    75.94 |    85.7 |   82.74 |                                               
 fabric.js |   83.02 |    75.94 |    85.7 |   82.74 | ...,30832,30906,30917-30982,31105,31204,31440 
-----------|---------|----------|---------|---------|-----------------------------------------------

@ShaMan123 ShaMan123 added the v6 label May 11, 2022
@ShaMan123 ShaMan123 changed the title refactor(object_interactivity): draw controls refactor(object_interactivity): draw borders May 14, 2022
@stale
Copy link

stale bot commented Jun 12, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Issue marked as stale by the stale bot label Jun 12, 2022
@ShaMan123 ShaMan123 removed the stale Issue marked as stale by the stale bot label Jun 13, 2022
* @param {Object} styleOverride object to override the object style
* @return {fabric.Object} thisArg
* @chainable
* @public override if necessary
Copy link
Member

Choose a reason for hiding this comment

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

we could say that this function is here exactly for the scope of overriding. Otherwise someone could argue, why having a method on a class that does not need this at all.
I would just say override this function in order to draw a different control box. example: rounded corners, different border style.
We should also inform whihc is the state of the context in that situation to give the dev some extra information.
(rotated aligned with object base, centered on the center of the object, scaled to absorb object scale and container scale and zoom )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ctx is in the same state as in the object's render method right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or maybe not. I am confused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(rotated aligned with object base, centered on the center of the object, scaled to absorb object scale and container scale and zoom )

hard to understand

Copy link
Member

Choose a reason for hiding this comment

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

the ctx is not in the same state, because when you scale the object the controls do not grow.
i m also wrong the ctx is only centered and rotated, and is not scaled at all.

I m pointing out that override in case of necessity can be writte on top of any function or none.
Since you made this refactor with a keen eye on having it overridable for some reason, just indicate the reason as an example for future reader

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I recall correctly it is rotated and translated to center. But not scaled or skewed...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@github-actions
Copy link
Contributor

github-actions bot commented Jun 16, 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  |   83.12 |    76.04 |   85.77 |   82.85 |                                               
 fabric.js |   83.12 |    76.04 |   85.77 |   82.85 | ...,30832,30906,30917-30982,31105,31204,31440 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Jun 16, 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  |   83.13 |    76.07 |   85.77 |   82.86 |                                               
 fabric.js |   83.13 |    76.07 |   85.77 |   82.86 | ...,30832,30906,30917-30982,31105,31204,31440 
-----------|---------|----------|---------|---------|-----------------------------------------------

@ShaMan123
Copy link
Contributor Author

READY

@asturur asturur merged commit 21003c7 into master Jun 16, 2022
@ShaMan123 ShaMan123 deleted the refactor-draw-controls branch September 11, 2022 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants