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

188286105 Drag from Plugins to Codap #1532

Merged
merged 34 commits into from
Oct 23, 2024
Merged

Conversation

tealefristoe
Copy link
Contributor

@tealefristoe tealefristoe commented Oct 2, 2024

PT Story: https://www.pivotaltracker.com/story/show/188286105

This work can be tested at: https://codap3.concord.org/branch/v3-di-drag-drop/#file=examples:Mammals
Using this version of Multidata: https://models-resources.concord.org/multidata-plugin/branch/drag-v3/index.html

This PR enables dragging attributes from plugins to Codap v3. In the past, this was accomplished using the HTML5 drag and drop system. However, Codap v3 uses dnd-kit for drag and drop, so the old system no longer works (and it never worked very well anyway, given security restrictions between iframes and their containers). Now, plugins enable drags within Codap by sending a series of API requests which trigger synthetic pointer events that dnd-kit reacts to.

  • Plugins cause drags in Codap v3 via notify attribute requests. Three requests must be made:
    • A dragStart request will begin a drag by creating synthetic mousedown and mousemove events (because a drag won't actually begin until the mouse is moved at least three pixels).
      • The size of the drag overlay should be specified in this request with overlayHeight and overlayWidth.
    • dragOver requests update the position of the drag overlay, and they should be continually requested as the user moves the mouse. mouseX and mouseY should be included in the request, and should be in the plugin's coordinate system.
    • A dragEnd request will end the drag. It should also include mouseX and mouseY.
  • PluginAttributeDrag is a dummy element that is used as the basis of plugin initiated drags. It's a transparent div in the Container.
  • An AttributeDragOverlay is also included in the Container, which follows the mouse during plugin initiated drags. Its size and position is controlled by API requests.
  • Information is passed from the API handler to the PluginAttributeDrag (dataSetId and attributeId) and AttributeDragOverlay (overlay size and position) via new uiState properties.
    • This feels inelegant to me, and I am open to alternate methods of achieving this. Using the synthetic events won't work.

Other changes in this PR:

  • A getOverlayDragId function is used to unify how AttributeDragOverlay activeDragIds are generated.
  • The new fieldRequiredResult function generates common error messages to API handlers.
  • Fix a bug in data-set-utils that failed to broadcast a notification in certain situations when attributes were moved.

@tealefristoe tealefristoe marked this pull request as draft October 2, 2024 21:39
@tealefristoe tealefristoe added the v3 CODAP v3 label Oct 2, 2024
Copy link

cypress bot commented Oct 2, 2024

codap-v3    Run #4737

Run Properties:  status check passed Passed #4737  •  git commit 20c21dcfb0: 188286105 Drag from Plugins to Codap (#1532)
Project codap-v3
Branch Review main
Run status status check passed Passed #4737
Run duration 01m 44s
Commit git commit 20c21dcfb0: 188286105 Drag from Plugins to Codap (#1532)
Committer Teale Fristoe
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 1
View all changes introduced in this branch ↗︎

Copy link

codecov bot commented Oct 9, 2024

Codecov Report

Attention: Patch coverage is 68.94410% with 50 lines in your changes missing coverage. Please review.

Project coverage is 84.32%. Comparing base (7281a65) to head (ab11b26).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
...src/data-interactive/handlers/attribute-handler.ts 14.81% 42 Missing and 4 partials ⚠️
...rc/components/drag-drop/attribute-drag-overlay.tsx 66.66% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1532       +/-   ##
===========================================
+ Coverage   54.69%   84.32%   +29.63%     
===========================================
  Files         577      584        +7     
  Lines       29133    29521      +388     
  Branches     8013     8044       +31     
===========================================
+ Hits        15935    24895     +8960     
+ Misses      12417     4293     -8124     
+ Partials      781      333      -448     
Flag Coverage Δ
cypress 73.93% <55.73%> (+55.71%) ⬆️
jest 52.45% <49.06%> (-0.39%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@kswenson kswenson left a comment

Choose a reason for hiding this comment

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

👍 Looks good -- just a few suggestions.

@@ -29,6 +30,11 @@ export function getDragAttributeInfo(active: Active | null): Omit<IDragAttribute
const { dataSet, attributeId } = active?.data.current as IDragAttributeData || {}
return dataSet && attributeId ? { dataSet, attributeId } : undefined
}
export function getOverlayDragId(active: Active | null, instanceId: string, excludeIndexColumn = false) {
Copy link
Member

Choose a reason for hiding this comment

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

To keep this function (and this module) more generic, couldn't the last argument be excludeId or excludeIds and then clients could pass kIndexColumnKey or [kIndexColumnKey] so there's no explicit dependence on case tiles here? It could even be a regex so the client is responsible for knowing whether to match the beginning/end, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I was working on converting this, I realized that CaseCard probably doesn't need to exclude the index id, and that its original inclusion was likely a copy/paste artifact. Does that sound right to you? If so I'll just remove the exception from the component.

v3/src/data-interactive/handlers/attribute-handler.ts Outdated Show resolved Hide resolved
v3/src/components/drag-drop/attribute-drag-overlay.tsx Outdated Show resolved Hide resolved
v3/src/components/drag-drop/attribute-drag-overlay.tsx Outdated Show resolved Hide resolved
v3/src/models/ui-state.ts Outdated Show resolved Hide resolved
@kswenson
Copy link
Member

One other comment I forgot to mention in my earlier review -- it would make sense to have constants for the ids/classes referenced in the DI code so they're less likely to get out of sync: e.g. plugin-attribute-drag, codap-container, codap-web-view-body.

@tealefristoe tealefristoe marked this pull request as draft October 22, 2024 22:26
@tealefristoe tealefristoe marked this pull request as ready for review October 22, 2024 23:41
Copy link
Member

@kswenson kswenson left a comment

Choose a reason for hiding this comment

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

👍 Changes look good! Just a couple minor suggestions.

v3/src/components/web-view/web-view-constants.ts Outdated Show resolved Hide resolved
v3/src/hooks/use-drag-drop.ts Outdated Show resolved Hide resolved
@tealefristoe tealefristoe merged commit 20c21dc into main Oct 23, 2024
15 of 16 checks passed
@tealefristoe tealefristoe deleted the 188286105-v3-di-drag-drop branch October 23, 2024 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants