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

fix(canvas_events): mouseout bug #8011

Merged
merged 4 commits into from
Jun 19, 2022
Merged

fix(canvas_events): mouseout bug #8011

merged 4 commits into from
Jun 19, 2022

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Jun 18, 2022

I have encountered a severe bug so I've allowed myself to PR disregarding the PR lockdown.
It's a simple fix, the bug seems to be a typo.

https://github.com/fabricjs/fabric.js/blame/master/src/mixins/canvas_events.mixin.js#L151

@fabricjs fabricjs deleted a comment from github-actions bot Jun 18, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jun 18, 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.19 |    76.17 |   85.84 |   82.92 |                                               
 fabric.js |   83.19 |    76.17 |   85.84 |   82.92 | ...,30842,30916,30927-30992,31115,31214,31450 
-----------|---------|----------|---------|---------|-----------------------------------------------

@ShaMan123 ShaMan123 added the bug label Jun 18, 2022
@ShaMan123 ShaMan123 requested a review from asturur June 18, 2022 06:59
@asturur
Copy link
Member

asturur commented Jun 18, 2022

bugfix are always fine

});
this.fire('mouse:out', { target: _target, e: e });
_target && _target.fire('mouseout', { e: e });
}, this);
Copy link
Member

Choose a reason for hiding this comment

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

since this was probably a distraction/typo as you say, we should name those things better.
I would call
https://github.com/fabricjs/fabric.js/pull/8011/files#diff-6011216a37d34f94eb2a73dbf67b8c7f58c35b42ba7e882da0aaac6e27225f28R144
as hoveredTarget, and _target hoveredNestedTarget.

What do you think?

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Jun 18, 2022 via email

@asturur
Copy link
Member

asturur commented Jun 18, 2022

HoveredTarget or target or the same IMO _target -> subTarget?? בתאריך שבת, 18 ביוני 2022, 17:09, מאת Andrea Bogazzi ‏< @.>:

@.
* commented on this pull request. ------------------------------ In src/mixins/canvas_events.mixin.js <#8011 (comment)>: > this._hoveredTargets.forEach(function(_target){ - _this.fire('mouse:out', { target: target, e: e }); - _target && target.fire('mouseout', { e: e }); - }); + this.fire('mouse:out', { target: _target, e: e }); + _target && _target.fire('mouseout', { e: e }); + }, this); since this was probably a distraction/typo as you say, we should name those things better. I would call https://github.com/fabricjs/fabric.js/pull/8011/files#diff-6011216a37d34f94eb2a73dbf67b8c7f58c35b42ba7e882da0aaac6e27225f28R144 as hoveredTarget, and _target hoveredNestedTarget. What do you think? — Reply to this email directly, view it on GitHub <#8011 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIGAW4NXP6GEV27EYBM64LTVPXKDDANCNFSM5ZEI7JLQ . You are receiving this because you authored the thread.Message ID: @.***>

Whatever that doesn't differ from a dash.
subTarget was a poor name choice from the start, those are indeed nested in the main target

@ShaMan123
Copy link
Contributor Author

Everywhere else it's called subTarget...
I pushed without the hovered prefix because it's obvious

@github-actions
Copy link
Contributor

github-actions bot commented Jun 19, 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.19 |    76.17 |   85.84 |   82.92 |                                               
 fabric.js |   83.19 |    76.17 |   85.84 |   82.92 | ...,30842,30916,30927-30992,31115,31214,31450 
-----------|---------|----------|---------|---------|-----------------------------------------------

@asturur asturur merged commit 9fbc85f into master Jun 19, 2022
@ShaMan123 ShaMan123 deleted the fix-mouse-out branch September 11, 2022 23:41
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