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) Remove small mixins that are too small to be worthy of existance #8542

Merged
merged 11 commits into from Dec 29, 2022

Conversation

asturur
Copy link
Member

@asturur asturur commented Dec 29, 2022

Motivation

Continue migration of TS

Description

Changes

  • BREAKING: remove canvas.straightenObject
  • BREAKING: cloneWithoutData clones only the size
  • BREAKING: canvas.clone now correctly return either a canvas or staticCanvas
  • 3 mixin file less.

Gist

In Action

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.

IMO canvas straightening belongs to animation
But actually we can dump it

@asturur
Copy link
Member Author

asturur commented Dec 29, 2022

all canvas animations are possibly dumped, are 4 premade animations that who needs them need probably more than 4 and can just make them

@github-actions
Copy link
Contributor

github-actions bot commented Dec 29, 2022

Build Stats

file / KB (diff) bundled minified
fabric 956.032 (-2.947) 305.797 (-0.961)

@github-actions
Copy link
Contributor

github-actions bot commented Dec 29, 2022

Coverage after merging remove-dataurl-mixin into master will be

82.79%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
HEADER.js54%48.15%0%63.64%12, 14, 14, 14, 14, 14, 16–17, 21, 21–22, 22, 22, 24, 24, 26, 28, 30–31, 77, 77, 77
src
   cache.ts97.06%90%100%100%56
   config.ts77.27%66.67%66.67%84.62%130, 138–140, 151–153
   constants.ts100%100%100%100%
   intersection.class.ts100%100%100%100%
   pattern.class.ts92.31%85.71%100%96.43%120, 126, 137, 146, 98
   point.class.ts100%100%100%100%
   shadow.class.ts98.51%96%100%100%157
src/brushes
   base_brush.class.ts100%100%100%100%
   circle_brush.class.ts1.52%0%0%2%101, 103–105, 114, 114, 114, 116, 118, 120–122, 124–127, 135, 142, 144, 24, 29–30, 38–42, 46–50, 57–60, 68–72, 74, 82, 82, 82, 82, 82–83, 85, 85, 85–88, 90, 98–99
   pattern_brush.class.ts97.14%87.50%100%100%22
   pencil_brush.class.ts91.91%85.42%100%93.64%123–124, 153, 153–155, 277, 281, 286–287, 69–70, 85–86
   spray_brush.class.ts1.16%0%0%1.56%100–102, 104–105, 113, 113, 113, 113, 113–114, 116–117, 124–125, 127, 129–133, 142, 146–147, 147, 155, 155, 155–158, 160–163, 167–168, 170, 172–175, 178, 185–186, 188, 190–191, 193, 200–201, 203–204, 207, 207, 214, 214, 218, 23–24, 26–28, 28, 28–30, 34, 43, 50, 57, 64, 71, 78, 90–92
src/canvas
   canvas.class.ts92.52%88.15%94.12%95.45%1151, 1151–1152, 1155, 1175, 1175, 1237, 1273–1274, 1293–1294, 1302–1303, 1324, 1332, 1445–1446, 1448–1449, 1469–1470, 1633–1634, 1638, 510, 577–578, 583, 593, 722–723, 725–726, 726, 726, 772–773, 834–835, 838, 840, 885–887, 917, 922–923, 952–953
   canvas_events.ts78.10%75%82.81%79.52%1003–1004, 1004, 1004–1006, 1008–1009, 1009, 1009, 1011, 1019, 1019, 1019–1021, 1021, 1021, 1027–1028, 1036–1037, 1037, 1037–1038, 1043, 1045, 1075–1077, 1080–1081, 1085–1086, 1184, 1204–1206, 1209–1210, 1282, 1369, 1402, 145, 1496–1497, 1503, 1507–1508, 1524, 1546, 1593, 1598, 1637, 170, 318–319, 319, 319–320, 320, 323–327, 332, 334, 346–348, 351, 368, 368, 368–369, 369, 369–370, 378, 383–384, 384, 384–385, 387, 396, 402–403, 403, 403, 439, 443, 443, 443, 443, 443–444, 446, 521, 523, 523, 523–525, 545, 547, 547, 547–548, 548, 548, 551, 551, 551–552, 555, 564–565, 567–568, 570, 570–571, 573–574, 586–587, 587, 587–588, 590–594, 600, 607, 647, 647, 647, 649, 651–655, 661, 667, 667, 667–668, 670, 673, 678, 691, 718, 774–775, 775, 775–776, 778, 781–782, 782, 782–783, 785–786, 789, 789–791, 794–795, 805, 887, 901, 908, 929, 961, 982–983, 989
   canvas_gestures.mixin.ts9.21%5.56%7.14%11.36%101, 114, 127, 139, 148, 157–161, 170, 172, 172, 172–173, 175–176, 189, 198, 207, 211, 30, 30, 30–31, 31, 31, 31, 36, 39–40, 40, 40–41, 47, 50, 58, 58, 58, 58, 58–59, 62–64, 66–67, 69, 71, 71, 71–73, 76, 78, 88
   static_canvas.class.ts92.54%85.99%98.04%95.27%1132–1133, 1133, 1133–1134, 1254, 1264, 1318–1319, 1322, 1357–1358, 1436, 1445, 1450, 1565, 1567, 1569, 1569, 1569, 1569, 1572, 1572, 1572–1573, 1625–1627, 1629, 1631, 1631, 1631, 1631, 1635, 1635, 1635–1637, 1690–1691, 1919, 1919–1920, 1970, 1973, 1976, 1976, 1976, 1979, 1982, 1982, 1982, 315, 347, 360, 416–417, 419–420, 429, 433–434, 436–437, 806, 806–807, 892
src/color
   color.class.ts91.67%84.51%100%94.44%325–326, 330–331, 334–335, 41, 45, 72–73, 73, 75, 75, 75–76, 78–79
   color_map.ts100%100%100%100%
   constants.ts100%100%100%100%
   index.ts100%100%100%100%
   util.ts100%100%100%100%
src/controls
   actions.ts100%100%100%100%
   changeWidth.ts100%100%100%100%
   control.class.ts93.98%88.89%90.91%97.78%236, 320, 320, 355
   controls.render.ts81.63%78%100%84.78%106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
   default_controls.ts86.67%66.67%100%100%125, 132
   drag.ts100%100%100%100%
   rotate.ts20%12.50%50%22.22%45, 51, 51, 51–52, 55–57, 59, 59, 59, 59, 59–61, 61, 61–63, 65, 65, 65–67, 67, 67–68,

@asturur
Copy link
Member Author

asturur commented Dec 29, 2022

removed straightenObject from canvas. Is not using any canvas property, is another:

objet.method() => method(object)

not worth anyone time

@asturur asturur marked this pull request as ready for review December 29, 2022 11:00
@asturur
Copy link
Member Author

asturur commented Dec 29, 2022

I think all the canvas animation can become functions that you can import to which you give a canvas and an object and you obtain an animaton.
Does not need to be on the instance, seems like something that was started and never really developed, and they look like more like 4 example animations to me
delete, bring to center ( x or y ), straighten

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.

I am for removing the animation mixin for good

@@ -1296,9 +1298,9 @@ export class StaticCanvas<
createSVGRefElementsMarkup(): string {
return ['background', 'overlay']
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return ['background', 'overlay']
return (['background', 'overlay'] as const)

will type the array and remove the need for the extra as 'overlayColor' | backgroundColor

Copy link
Contributor

Choose a reason for hiding this comment

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

ShaMan123
ShaMan123 previously approved these changes Dec 29, 2022
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.

see my comments

src/canvas/static_canvas.class.ts Show resolved Hide resolved
el.width = this.width;
el.height = this.height;
// this seems wrong. either Canvas or StaticCanvas
return new StaticCanvas(el);
Copy link
Contributor

Choose a reason for hiding this comment

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

you can do

new this.constructor(el)

Then it takes the instance class

Copy link
Member Author

Choose a reason for hiding this comment

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

tried already , is not happy about it. Neither with new this(el) nor with new this.contrsuctor(el).

@asturur asturur merged commit aff3916 into master Dec 29, 2022
@asturur asturur deleted the remove-dataurl-mixin branch December 29, 2022 15:23
frankrousseau pushed a commit to cgwire/fabric.js that referenced this pull request Jan 6, 2023
Co-authored-by: ShaMan123 <shacharnen@gmail.com>
ShaMan123 added a commit that referenced this pull request Mar 16, 2024
Co-authored-by: ShaMan123 <shacharnen@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