Skip to content

Comments

pass a callback to handle Drag and Drop state changes#46

Merged
KetanReddy merged 3 commits intoplayer-ui:feature/drag-and-dropfrom
chengliwang:feat-pass-a-callback-function-to-handle-Drag-and-Drop-state-changes
Feb 23, 2023
Merged

pass a callback to handle Drag and Drop state changes#46
KetanReddy merged 3 commits intoplayer-ui:feature/drag-and-dropfrom
chengliwang:feat-pass-a-callback-function-to-handle-Drag-and-Drop-state-changes

Conversation

@chengliwang
Copy link

pass a callback to handle Drag and Drop state changes

@codecov
Copy link

codecov bot commented Feb 22, 2023

Codecov Report

Base: 54.89% // Head: 55.08% // Increases project coverage by +0.19% 🎉

Coverage data is based on head (bbdf3fb) compared to base (94ed219).
Patch coverage: 83.33% of modified lines in pull request are covered.

Additional details and impacted files
@@                    Coverage Diff                    @@
##           feature/drag-and-drop      #46      +/-   ##
=========================================================
+ Coverage                  54.89%   55.08%   +0.19%     
=========================================================
  Files                         64       64              
  Lines                       3323     3342      +19     
  Branches                     977      985       +8     
=========================================================
+ Hits                        1824     1841      +17     
- Misses                      1174     1176       +2     
  Partials                     325      325              
Impacted Files Coverage Δ
drag-and-drop/library/src/utils/helpers.ts 85.71% <82.60%> (-14.29%) ⬇️
...g-and-drop/library/src/utils/runtime-flow-state.ts 52.94% <83.33%> (+3.47%) ⬆️
drag-and-drop/library/src/controller.tsx 48.64% <100.00%> (-12.37%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

);
}

exportContent(): View {
Copy link
Member

Choose a reason for hiding this comment

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

Why move this from the controller to here?

Copy link
Member

Choose a reason for hiding this comment

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

Also can you mark this as either public or private

this.realAssetMappings.delete(assetSymbol);
parentDropTarget.value = this.computeViewForDropTarget(parentDropTarget);

this.handleDndStateChange(this.exportContent());
Copy link
Member

Choose a reason for hiding this comment

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

To make it a bit cleaner, Instead of calling the passed in function directly here with arguments, there should be a local function that does any of the necessary pre-processing and then called the passe in function.

@@ -507,33 +527,38 @@ export class RuntimeFlowState {

createDropTarget(
Copy link
Member

@KetanReddy KetanReddy Feb 22, 2023

Choose a reason for hiding this comment

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

This should be private for now

const baseView = this.view;

/** Walks the drag and drop state to remove any drop target assets */
const removeDndStateFromView = (obj: unknown): any => {
Copy link
Member

Choose a reason for hiding this comment

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

This might be better to define in a utility file or something since it is static and doesn't really need to be in the flow state manager.

Copy link
Author

Choose a reason for hiding this comment

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

extracted exportContent to a helper function

@KetanReddy KetanReddy merged commit de99318 into player-ui:feature/drag-and-drop Feb 23, 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.

2 participants