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() BREAKING: align line to other classes for positioning #8877

Merged
merged 7 commits into from May 6, 2023

Conversation

asturur
Copy link
Member

@asturur asturur commented Apr 29, 2023

Motivation

Line has some peculiar code that could be removed in favor of standard methods we use everyewhere else.

Description

Trying to remove the custom code for the standard setPostionByOrigin and makeBoundingBoxFromPoints, leaving open the issue with changes in positioning.

This won't affect old saved data but affect lines that are created without left,top and just points.
I would seem normal that if i have a line that extends from 5,15 to 15,5 it's center is in 10, 10 regardless of strokeWidth and that the left, top and originX or originY are assigned depending from the options or the defaults but not from the points themselves

Today you get different originX and originY depending on the order of your points, like if x1 or y1 are the origin, and unless you specified a different origin.

Make sense to remove those contraddictions but there are cases in which the lines aren't anymore in the previous places when initialized with the same points.

Gist

In Action

@github-actions
Copy link
Contributor

github-actions bot commented Apr 29, 2023

Build Stats

file / KB (diff) bundled minified
fabric 903.290 (-0.462) 304.817 (-0.521)

@ShaMan123
Copy link
Contributor

I know you don't want more changes...
However, why not to completely remove this or make a subclass of Polyline, overriding toObject/fromObject/fromElement?

@asturur
Copy link
Member Author

asturur commented Apr 30, 2023

Because you can already have a polyline with 2 points.
I don't like this class but i don't want to risk svg import now.
We would need to keep everything and what would go away is basically the render code for the line and nothing more.

@asturur
Copy link
Member Author

asturur commented May 1, 2023

Ideally both polyon, polyline and line just extend Path limiting some functionality, but we need a smart way to handle the points handling afterwards.

@asturur
Copy link
Member Author

asturur commented May 1, 2023

I m amazed that the more es6 i use, the more the bundle grow, i think is all safari fault.
Yes safari become useful from version 13.
Version 13 is probably safe as a base version, but i can't find stats.
Also pretty much useless if then popular phone browsers do not support modern js

@ShaMan123
Copy link
Contributor

It is really strange the stats from the last prs.
I don't pay enough attention to that.
Just see readable es6 code and smile...

@github-actions
Copy link
Contributor

github-actions bot commented May 1, 2023

Coverage after merging delete-line-code into master will be

83.73%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.node.ts7.69%100%0%14.29%17, 20, 23, 29, 32, 35
src
   ClassRegistry.ts100%100%100%100%
   Collection.ts94.71%94.64%86.67%97.09%101, 104, 207–208, 233–234
   CommonMethods.ts96.55%87.50%100%100%10
   Intersection.ts100%100%100%100%
   Observable.ts87.23%85.29%84.62%89.36%144–145, 170–171, 39–40, 48, 57, 91, 99
   Pattern.ts92.21%91.89%90%93.33%114, 138, 149, 158, 51
   Point.ts100%100%100%100%
   Shadow.ts98.36%95.65%100%100%178
   cache.ts97.06%90%100%100%57
   config.ts75%66.67%66.67%82.76%130, 138, 138, 138, 138, 138–140, 151–153
   constants.ts100%100%100%100%
   typedefs.ts100%100%100%100%
src/brushes
   BaseBrush.ts100%100%100%100%
   CircleBrush.ts0%0%0%0%100, 102–104, 113, 113, 113, 115, 117, 119–121, 123–126, 133–134, 141, 143, 28–29, 37–41, 45–49, 56–59, 67–71, 73, 81, 81, 81, 81, 81–82, 84, 84, 84–87, 89, 97–98
   PatternBrush.ts97.06%87.50%100%100%21
   PencilBrush.ts91.01%82.35%100%93.75%122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
   SprayBrush.ts0%0%0%0%100–101, 103–104, 112, 112, 112, 112, 112–113, 115–116, 123–124, 126, 128–132, 141, 145–146, 146, 154, 154, 154–157, 159–162, 166–167, 169, 171–174, 177, 184–185, 187, 189–190, 192, 199–200, 202–203, 206, 206, 213, 213, 217, 22–23, 25–27, 27, 27–29, 33, 42, 49, 56, 63, 70, 89–91, 99
src/canvas
   Canvas.ts78.98%77.54%81.67%79.60%1000–1001, 1001, 1001–1003, 1005–1006, 1006, 1006, 1008, 1016, 1016, 1016–1018, 1018, 1018, 1024–1025, 1033–1034, 1034, 1034–1035, 1040, 1042, 1073–1075, 1078–1079, 1083–1084, 1197–1199, 1202–1203, 1276, 1395, 1518, 1588, 162, 187, 296–297, 300–304, 309, 332–333, 338–343, 363, 363, 363–364, 364, 364–365, 37, 373, 378–379, 379, 379–380, 382, 391, 397–398, 398, 398, 41, 441, 449, 453, 453, 453–454, 456, 538–539, 539, 539–540, 546, 546, 546–548, 568, 570, 570, 570–571, 571, 571, 574, 574, 574–575, 578, 587–588, 590–591, 593, 593–594, 596–597, 609–610, 610, 610–611, 613–617, 623, 630, 670, 670, 670, 672, 674–678, 684, 690, 690, 690–691, 693, 696, 701, 714, 741, 741, 799–800, 800, 800–801, 803, 806–807, 807, 807–808, 810–811, 814, 814–816, 819–820, 890, 902, 909, 930, 962, 983–984
   SelectableCanvas.ts94.39%91.16%94.55%96.63%1112, 1112–1113, 1116, 1136, 1136, 1194, 1247–1248, 1269, 1277, 1402, 1404, 1406–1407, 511, 691–692, 694–695, 695, 695, 744–745, 806–807, 860–862, 894, 899–900, 927–928
   StaticCanvas.ts96.01%91.48%100%97.93%1128–1129, 1129, 1129–1130, 1250, 1260, 1314–1315, 1318, 1353–1354, 1431, 1440, 1440, 1444, 1444, 1491–1492, 1706–1707, 327–328, 345, 425–426, 428–429, 790, 802–803, 888
   TextEditingManager.ts100%100%100%100%
src/color
   Color.ts92.16%86.49%100%94.29%330–331, 335–336, 339–340, 58, 88–89, 89, 91, 91, 91–92, 94–95
   color_map.ts100%100%100%100%
   constants.ts100%100%100%100%
   util.ts100%100%100%100%
src/controls
   Control.ts93.90%88.89%90.91%97.73%235, 322, 322, 357
   changeWidth.ts100%100%100%100%
   commonControls.ts100%100%100%100%
   controlRendering.ts81.63%78%100%84.78%106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
   drag.ts100%100%100%100%
   polyControl.ts5.97%0%0%11.11%100, 105, 119, 119, 119, 119, 119, 121–124, 124, 127, 134, 17, 25–29, 29, 29, 29, 29, 29, 29, 29, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–83, 83, 83–84, 88–90, 90, 90, 90, 90, 92
   rotate.ts19.57%12.50%50%21.43%41, 45, 51, 51, 51–52, 55–57, 59, 59, 59, 59, 59–61, 61, 61–63, 65, 65, 65–67, 67, 67–68, 73, 73, 73–74, 76, 78, 80–81
   scale.ts93.57%92.94%100%93.67%129–130, 132–134, 148–149, 181–183, 42
   scaleSkew.ts78.79%64.29%100%85.71%27, 29, 29, 29, 31, 33, 35
   skew.ts91.03%79.31%100%97.67%130–131, 162–163, 170, 176, 178
   util.ts100%100%100%100%
   wrapWithFireEvent.ts100%100%100%100%
   wrapWithFixedAnchor.ts100%100%100%100%
src/env
   browser.ts84.21%77.78%50%100%14, 17
   index.ts100%100%100%100%
   node.ts76.92%50%66.67%88.24%27, 31–32, 32, 32
src/filters
   BaseFilter.ts21.93%22.81%32%19.05%106–109, 109, 109–110, 116, 116, 116–119, 137, 155, 169–174, 178–179, 179, 179–182, 182, 182,

@asturur
Copy link
Member Author

asturur commented May 1, 2023

is just because every time we add a deconstruct in the arguments safari freaks out.
Is ok one day we can move to safari 13 and the bundle will shrink.

@asturur
Copy link
Member Author

asturur commented May 1, 2023

you should see the template with parameters what does it do:

    return ['<line ', 'COMMON_PARTS', "x1=\"".concat(x1, "\" y1=\"").concat(y1, "\" x2=\"").concat(x2, "\" y2=\"").concat(y2, "\" />\n")];

@ShaMan123
Copy link
Contributor

ShaMan123 commented May 1, 2023

Web dev can't just go smoothly even when things start to workout

@ShaMan123
Copy link
Contributor

Web dev can just go smoothly even when things start to workout

Can't

@asturur asturur changed the title Remove some goofy code for Line refactor() aling line to other classes for positioning May 6, 2023
@asturur asturur changed the title refactor() aling line to other classes for positioning refactor() BREAKING: align line to other classes for positioning May 6, 2023
@asturur asturur merged commit d37c7f7 into master May 6, 2023
18 checks passed
@asturur asturur deleted the delete-line-code branch May 6, 2023 23:02
Comment on lines +88 to +93
this.width = Math.abs(x2 - x1);
this.height = Math.abs(y2 - y1);
const { left, top, width, height } = makeBoundingBoxFromPoints([
{ x: x1, y: y1 },
{ x: x2, y: y2 },
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

redundant calc of width/height
obtain from makeBoundingBoxFromPoints

@asturur asturur mentioned this pull request May 19, 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.

None yet

2 participants