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

feat(Object): pass ctx to dnd methods #9844

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

ShaMan123
Copy link
Contributor

Description

Control over the context the dnd effects are drawn upon is vital for apps.
Also since we transform it beforehand it must be supplied

In Action

Copy link

codesandbox bot commented May 2, 2024

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

Copy link
Contributor

github-actions bot commented May 2, 2024

Build Stats

file / KB (diff) bundled minified
fabric 918.407 (+1.535) 306.077 (+0.422)

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.

ready for review, please make this part of rc

src/shapes/IText/IText.ts Outdated Show resolved Hide resolved
src/shapes/IText/IText.ts Outdated Show resolved Hide resolved
@ShaMan123 ShaMan123 requested a review from asturur May 2, 2024 16:51
@asturur
Copy link
Member

asturur commented May 3, 2024

The drag/drop effects are relatively new, if you want to swap the order of the arguments do it, but think first if to do an object or more paramters.
I tagged RC already but i don't mind is not a conceptual change this one

@asturur asturur changed the title chore(Object): pass ctx to dnd methods feat(Object): pass ctx to dnd methods May 3, 2024
@asturur
Copy link
Member

asturur commented May 3, 2024

Either merge as is or re-check after param swapping

@ShaMan123
Copy link
Contributor Author

I prefer param switching, do you think renderDrag.DropEffect should have ctx as first param as well?

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented May 3, 2024

The drag/drop effects are relatively new, if you want to swap the order of the arguments do it, but think first if to do an object or more paramters. I tagged RC already but i don't mind is not a conceptual change this one

I missed this message. I want your opinion. I will change renderCursorAt and will wait for your call on the rest

@ShaMan123 ShaMan123 force-pushed the chore/dnd-ctx branch 2 times, most recently from b746927 to 4452438 Compare May 3, 2024 12:03
Copy link
Contributor

github-actions bot commented May 3, 2024

Coverage after merging chore/dnd-ctx into master will be

82.59%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.node.ts14.29%100%0%25%23, 26, 29, 41, 44, 47
src
   ClassRegistry.ts88.71%53.85%100%97.73%31, 52, 52
   Collection.ts71.39%31.15%45.16%83.91%130, 138, 153, 155–157, 159, 169–170, 180–181, 197, 215, 217, 219, 228, 243, 254, 263, 265, 270, 279, 281, 283, 286–287, 302, 304, 306, 309–310, 327–328, 328, 328–329, 333–334, 337–344, 346–348, 350, 75, 99
   CommonMethods.ts85.51%61.54%66.67%94%50, 52, 61, 9
   Intersection.ts81.42%41.30%73.33%96.14%105, 132–133, 166, 184–190, 228, 228, 237, 239, 289–291, 297, 297, 78, 80
   Observable.ts71.20%45.45%31.25%85.48%136, 145, 148, 160, 162, 165, 167, 41, 43, 68–70, 72, 76, 78, 80, 82–85, 87–91
   Point.ts84.43%61.22%41.18%92.38%104, 117, 148, 157, 179, 197, 206, 216, 225, 236–239, 259, 284–285, 294–297, 317, 328, 341, 349, 359, 95
   Shadow.ts79.76%17.39%33.33%88.37%146–147, 147–150, 152–157, 166, 183, 183, 201, 201, 203–204, 204, 206, 213, 213, 230–237, 239, 239, 241–242, 38–41
   cache.ts83.72%45.45%75%90.14%57, 59, 71–72, 74–77
   config.ts82.21%20%44.44%94.03%123, 131–132, 134–137, 139, 142–143, 147, 152, 152, 152, 152, 152, 152
   constants.ts100%100%100%100%
src/LayoutManager
   ActiveSelectionLayoutManager.ts86.67%76.92%28.57%100%
   LayoutManager.ts89%65.06%53.85%99.29%269, 333, 344
   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.ts69.23%33.33%100%78.95%39, 41–42, 42–44, 46–48, 57, 57–58, 58, 66–69
   FitContentLayout.ts100%100%100%100%
   FixedLayout.ts85.71%20%100%100%23, 23
   LayoutStrategy.ts86.78%55.56%71.43%97.70%54, 72, 74, 88
   utils.ts62.90%7.14%100%78.72%28–32, 34–35, 39, 39–44, 47, 47, 47
src/Pattern
   Pattern.ts54.39%3.03%10%65.95%105–107, 113, 113–114, 117, 117–119, 119, 119–122, 130, 130–132, 132, 132, 132, 132, 132–138, 140–141, 143, 150, 150, 150, 150, 153–160, 160, 160–164, 174–175, 175, 175, 175–176, 176, 176–182, 182, 182, 182–183, 183, 183–188, 190–199, 204–205, 207–209, 211, 33, 37
src/brushes
   BaseBrush.ts78.67%8.33%14.29%88.55%110, 114, 114–115, 122, 122, 122, 122, 124–125, 130, 135, 143, 146, 155, 155, 155, 155, 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.ts72.74%30.88%76.79%84.67%1004, 1008, 1012–1013, 1018, 1047, 1049, 1049, 1056, 1056, 1062–1068, 1074, 1105, 1126, 1130, 1137, 1140, 117, 1203–1207, 1241, 1260, 1286–1290, 1293, 1296, 1314, 1320, 1335, 1337, 1383–1404, 1406–1407, 1407–1411, 1414–1417, 1421–1424, 1426–1428, 1432, 1434, 1436–1438, 1441–1446, 1449–1451, 1454, 1456, 1470, 1477, 1479–1495, 1495, 1495, 1497, 1501–1502, 1505–1506, 1509–1511, 1514, 1522, 183, 318, 323, 343, 354, 369, 388, 431, 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, 703, 747, 783–786, 789, 791, 791, 794, 796, 798, 824, 830, 839–840, 844, 867, 870, 870, 887, 906, 932–933, 936, 938, 948–953, 956, 964, 964–970, 972, 974, 994–996, 999
   CanvasOptions.ts100%100%100%100%
   SelectableCanvas.ts83%49.39%80%93.49%1021, 1089–1093, 1139–1140, 1142–1143, 1145, 1172, 1174–1175, 1175, 1177, 1213, 1216, 1218, 1220, 1236, 1238, 1266, 1269, 1288, 1290,

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.

I heeded your advice after I understood #9850
Passing points and source/target make sense

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.

This is ready for another review
Done on my end

changed old chore to feat
@asturur
Copy link
Member

asturur commented May 5, 2024

Please keep them separate.

My suggestion was saying that are probably multiple way to fix it this one seem the one with more code added.
It could be possible to just change scenePoint in something that is uncached and use the other cached value that doesn't change with the viewport.
That would be a 2 line change but we should check if it has impact in performance.

Also in this PR i think there is more that for the events? i see a prevTarget that seems new.

@ShaMan123
Copy link
Contributor Author

I couldn't understand your comment
I passed prevDropTarget to _renderDragEffects to keep the code straightforward and supply better context for the API

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