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(): use deconstruction and constants in place of strings to save some bytes of code #9593

Merged
merged 14 commits into from
Jul 2, 2024

Conversation

asturur
Copy link
Member

@asturur asturur commented Jan 13, 2024

Description

This is no functional changes a tentative to recover some bundle size

In Action

Copy link

codesandbox bot commented Jan 13, 2024

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

Copy link
Contributor

github-actions bot commented Jan 13, 2024

Build Stats

file / KB (diff) bundled minified
fabric 913.112 (+0.228) 304.542 (-0.269)

@asturur asturur changed the title chore(): use deconstruction to save some bytes of code chore(): use deconstruction and constants in place of strings to save some bytes of code Jan 13, 2024
@asturur asturur marked this pull request as draft January 14, 2024 07:59
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.

In an ideal condition I would ask this to be merged once our queue is empty to avoid annoying merge conflicts with open PRs because it is a lot of diff ink

But we are not there yet so it is what it is

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the fate of this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

at some point we need to find a solution for gestures

Copy link
Contributor

Choose a reason for hiding this comment

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

I gave some thought.
We can introduce a control that fits the entire object for a 2 finger gesture that scales and rotates the object.
The math sounds straight forward.
Measuring the diff of the vector created between the 2 touches.
Then the dev can do for 3 fingers whatever based on the 2 finger control.
Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would then argue that a mobile/touch app will not want the default controls

Copy link
Contributor

Choose a reason for hiding this comment

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

Then touch become a first class citizen in fabric

src/constants.ts Outdated
export const ROTATE = 'rotate';
export const SKEWING = 'skewing';
export const RESIZING = 'resizing';
export const MODIFYPOLY = 'modifypoly';
Copy link
Contributor

Choose a reason for hiding this comment

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

can we rename this?

Comment on lines 29 to 33
return isAlternative ? 'skewX' : 'scaleY';
return isAlternative ? 'skewX' : SCALE_Y;
}
if (control.y === 0) {
// then is scaleY or skewX
return isAlternative ? 'skewY' : 'scaleX';
Copy link
Contributor

Choose a reason for hiding this comment

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

skewX/Y?

@asturur
Copy link
Member Author

asturur commented Jan 14, 2024

this can wait a lot is not important, i more wanted to see how much margin there was for improvements with strings

@asturur asturur marked this pull request as ready for review July 2, 2024 23:35
Copy link
Contributor

github-actions bot commented Jul 2, 2024

Coverage after merging investigate-layoutmanager-binding into master will be

84.90%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.node.ts14.29%100%0%25%23, 26, 29, 41, 44, 47
src
   ClassRegistry.ts90.32%61.54%100%97.73%31, 52
   Collection.ts78.47%42.62%87.10%85.82%130, 138, 153, 155–157, 159, 169–170, 181, 197, 215, 217, 228, 243, 254, 265, 270, 279, 281, 286–287, 302, 304, 309–310, 329, 333–334, 338–344, 346–348, 350
   CommonMethods.ts91.43%71.43%100%96%50, 52
   Intersection.ts85.25%48.91%100%97.30%184–188, 190, 228, 237, 239, 289, 297, 297
   Observable.ts79.89%54.55%93.75%87.10%136, 145, 148, 160, 162, 167, 68–70, 72, 76, 80, 84–85, 87–91
   Point.ts90.27%61.22%100%93.60%104, 117, 148, 157, 179, 197, 206, 216, 225, 236–239, 259, 285, 297, 317, 328, 341, 349, 359, 95
   Shadow.ts89.39%78.26%100%90.19%145, 148, 150–155, 164, 201, 204, 211, 228–235, 239–240
   cache.ts84.88%45.45%100%90.14%57, 59, 71–72, 74–77
   config.ts87.73%55%66.67%94.03%132, 134–137, 139, 142–143, 147, 152
   constants.ts100%100%100%100%
src/LayoutManager
   ActiveSelectionLayoutManager.ts93.33%76.92%85.71%100%
   LayoutManager.ts90.68%65.06%76.92%99.31%277, 342, 353
   constants.ts100%100%100%100%
   index.ts48.57%37.50%80%66.67%1, 1, 1–2, 2, 2–3, 3, 3–4, 4, 4–5, 5, 5, 5–6
   types.ts100%100%100%100%
src/LayoutManager/LayoutStrategies
   ClipPathLayout.ts75%56.25%100%78.95%39, 41–44, 46–48, 66–69
   FitContentLayout.ts100%100%100%100%
   FixedLayout.ts85.71%20%100%100%23, 23
   LayoutStrategy.ts87.50%55.56%100%96.51%46, 54, 73, 73, 75
   utils.ts72.58%50%100%78.72%29–32, 34–35, 40–44
src/Pattern
   Pattern.ts70.18%90.91%80%65.95%105–107, 114, 118–119, 119–122, 130–138, 140–141, 143, 153–164, 174, 176–181, 183–188, 190–199, 204–205, 207–209, 211, 33, 37
src/brushes
   BaseBrush.ts89.33%91.67%100%88.55%110, 115, 124–125, 130, 135, 143, 146, 155–160, 99
   CircleBrush.ts52.10%12.50%12.50%58.25%100–108, 108–118, 122, 130–139, 55, 67, 69, 76, 76, 78–79, 79, 83, 85–86, 92–98
   PatternBrush.ts97.06%87.50%100%100%21
   PencilBrush.ts91.06%82.35%100%93.81%122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
   SprayBrush.ts0%0%0%0%107, 107, 107, 107, 107–108, 110–111, 118–119, 121, 123–127, 136, 140–141, 141, 148, 148, 148–151, 153–156, 160–161, 163, 165–168, 17, 171, 178–179, 18, 181, 183–184, 186, 193–194, 196–197, 20, 200, 200, 207, 207, 21, 211, 22, 22, 22–24, 28, 32, 39, 46, 53, 60, 67, 84–86, 94–96, 98–99
src/canvas
   Canvas.ts75.45%39.53%92.86%85%1004, 1007–1008, 1012–1013, 1018, 1063–1068, 1105, 1126, 1128, 1130, 1137, 1140, 1203–1207, 1286–1290, 1317, 1334, 1380–1397, 1403–1408, 1411–1412, 1414, 1418, 1420–1421, 1423–1425, 1429, 1431, 1433–1435, 1438–1443, 1446–1448, 1451, 1453, 1467, 1474, 1476–1487, 1489–1492, 1492, 1494, 1498–1499, 1502–1503, 1506–1508, 1511, 328, 353, 369, 388, 443, 559–563, 566–567, 569, 579, 582–583, 585, 588–590, 602, 609–613, 615–620, 622–626, 659, 661, 668–672, 674–679, 681, 683–684, 686–689, 691–692, 747, 783–786, 789, 791, 794, 796, 798, 824, 886–887, 932–933, 935–936, 938, 947–953, 956, 963–964, 966–970, 972, 974, 995–996, 999
   CanvasOptions.ts100%100%100%100%
   SelectableCanvas.ts85.36%53.68%100%94.28%1006, 1008–1009, 1026, 1035, 1103–1107, 1153–1154, 1156–1157, 1159, 1186, 1188–1189, 1189, 1191, 1230, 1232, 1234, 1250, 1252, 1256, 1259, 1280, 1283, 1326–1333, 1336, 1339, 1348–1349, 1353–1362, 1366, 1368–1369, 1375–1380, 1384, 340–342, 346, 376, 398, 476, 527, 529, 605, 610, 687, 715
   StaticCanvas.ts75.74%69.50%98.91%75.30%1005, 1008, 1010, 1012, 1020, 1022–1028, 1038, 1041, 1044–1050, 1052–1053, 1055–1076, 1083–1085, 1087, 1095, 1101–1103, 1103–1104, 1104–1105, 1107, 1107–1112, 1125, 1130–1132, 1136, 1140, 1142, 1144, 1149–1153, 1155, 1157–1160, 1163, 1165, 1174, 1176–1177, 1184–1187, 1189, 1195–1198, 1202–1203, 1213, 1219–1221, 1223–1228, 1228, 1228–1232, 1232, 1232–1237, 1239–1246, 1275–1276, 1278–1283, 1285–1291, 1295, 1297–1310, 1318–1319, 1329, 1340, 1381–1385, 1387, 1389, 1391–1395, 1414, 1430, 1447, 1467, 1500, 1504, 1515–1517, 424, 435, 443–448, 456,

@asturur asturur merged commit 307b40d into master Jul 2, 2024
20 checks passed
@asturur asturur deleted the investigate-layoutmanager-binding branch July 2, 2024 23:47
This pull request was closed.
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