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): IText behavior mixins #8421

Merged
merged 63 commits into from
Nov 20, 2022
Merged

chore(TS): IText behavior mixins #8421

merged 63 commits into from
Nov 20, 2022

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Nov 3, 2022

Motivation

Description

Continues #8420
This PR migrates IText mixins into classes that are chained to create the final IText class.
I have treated itext_behavior as the base class, holding more general methods, while itext_key_behavior and itext_click_behavior hold specific methods. itext_key_behavior is also in charge of the hidden textarea since it is part of the key behavior scope.
I moved some methods around to accommodate my approach but this is far from perfect.
I would rename the classes as well.
I have used abstract methods to make it easier to migrate the mixins. A follow up should do a heavier refactor, moving more methods and making the mixins more defined in scope.

Changes

Gist

Proto chain as follows:

  1. itext_behavior extends Text
  2. itext_key_behavior
  3. itext_click_behavior
  4. ITEXT

In Action

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Nov 3, 2022

Build stats are wrong
The bundle was 93.x
FIXED by 1d9c4ed

Build Stats

file / KB (diff) bundled reduced* minified gzipped
fabric.min.js 1078.104 (+63.882) 331.222 (+30.636) 93.176 (+7.604)
src/mixins/itext_behavior.mixin.ts 43.055 (+43.055) 44.184 (+44.184)
src/mixins/itext_click_behavior.mixin.ts 9.402 (+9.402) 9.602 (+9.602)
src/mixins/itext_key_behavior.mixin.ts 19.997 (+19.997) 19.050 (+19.050)
src/mixins/object_interactivity.mixin.ts 20.364 (+0.001) 18.729 (0.000)
src/mixins/observable.mixin.ts 4.599 (+0.001) 3.374 (0.000)
src/mixins/text_style.mixin.ts 9.339 (+9.339) 9.283 (+9.283)
src/shapes/itext.class.ts 19.490 (-0.741) 18.364 (-2.820)
src/shapes/text.class.ts 54.621 (-5.601) 50.654 (-12.394)
src/shapes/textbox.class.ts 14.160 (-1.123) 14.104 (-3.030)
*inaccurate, see link

ShaMan123 added a commit that referenced this pull request Nov 4, 2022
@asturur
Copy link
Member

asturur commented Nov 20, 2022

Tried my best for conflicts, let's see.
Decription sucks.

@asturur
Copy link
Member

asturur commented Nov 20, 2022

28kb what? is that broken again?

@ShaMan123
Copy link
Contributor Author

Updated description

@ShaMan123
Copy link
Contributor Author

b94dff4 fixed the wrong stats

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Nov 20, 2022

2kb less 🥳 after minification!

ShaMan123 added a commit that referenced this pull request Nov 20, 2022
once #8421 is merged I will bind text events to IText
@asturur
Copy link
Member

asturur commented Nov 20, 2022

So we know that text mixins are messy. I don't think right now renaming classes/mixins/files is a good idea.
Important is that we type what is typeable.
There is code that can probalby improved in the text interaction area, let's collect ideas in a doc with examples and let's move one. is on par or better than before and that is what it count since this is a tech migration

Copy link
Contributor Author

@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.

Agreed

@asturur asturur merged commit bfae2d6 into master Nov 20, 2022
@asturur
Copy link
Member

asturur commented Nov 20, 2022

@ShaMan123 i ll start Static canvas today, and i look forward merging Image and observable types when we can.
I also know you probably want to look at group?

@ShaMan123
Copy link
Contributor Author

Yep. Started group. Will try to fix the failing test in Image.
What about the animation pr?

@asturur asturur deleted the ts-texts-mixins branch November 27, 2022 00:15
frankrousseau pushed a commit to cgwire/fabric.js that referenced this pull request Jan 6, 2023
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.

2 participants