feat: support remove scope #143

sudongyuer · 2022-12-13T15:34:47Z

lihebi · 2022-12-13T20:35:48Z

ui/src/components/Canvas.tsx

+    if(node?.parentNode && !node.position.x && dragging){
+      node.parentNode = undefined
+      node.data!.parent = 'ROOT'
+      setPodParent({ id, parent: 'ROOT', dirty: false });
+    }


Two issues:

this allows only dragging a pod out of a scope from left side, but not other 3 sides. This behavior sounds inconsistent.

we need to update the position (which is relative to the parent scope), otherwise, the pod will "jump" upon moving out.

More broadly speaking, we probably want another way to dissociate the pod from the scope, other than simply dragging, because the current pod-cannot-be-moved-out behavior is useful. We could probably add a "release button" which, once clicked, temporarily remove extent: 'parent' field to allow it to be moved out.

See more discussion on issue #99

lihebi · 2022-12-13T20:36:06Z

ui/src/components/Canvas.tsx

+            {/*<MiniMap*/}
+            {/*  nodeStrokeColor={(n) => {*/}
+            {/*    if (n.style?.borderColor) return n.style.borderColor;*/}
+            {/*    if (n.type === "code") return "#d6dee6";*/}
+            {/*    if (n.type === "scope") return "#f4f6f8";*/}
+
+            {/*    return "#d6dee6";*/}
+            {/*  }}*/}
+            {/*  nodeColor={(n) => {*/}
+            {/*    if (n.style?.backgroundColor) return n.style.backgroundColor;*/}
+
+            {/*    return "#f4f6f8";*/}
+            {/*  }}*/}
+            {/*  nodeBorderRadius={2}*/}
+            {/*/>*/}


Why do you remove the minimap?

Because i don't know many context about the source code , i just remove them for debug , when i make this pr ready for review , i will reset them

Ah, I see, sounds good!

lihebi · 2022-12-13T20:36:28Z

ui/src/components/Canvas.tsx

+            {/*<Controls showInteractive={role !== RoleType.GUEST} />*/}
+
+            {/*<Background />*/}


and the controls and the canvas backgrounds?

Thanks, sudongyuer! There appear to be a few coding and design issues.

Yes , so i make this pr to Draft

lihebi

Thanks, sudongyuer! There appear to be a few coding and design issues.

…pod-io#148)

sudongyuer · 2022-12-15T03:27:29Z

This is this PR's behavior

Screen.Recording.2022-12-15.at.11.25.26.mov

forrestbao · 2022-12-15T04:19:15Z

@sudongyuer What will happen if you move the scope after the code cell has been moved out of it? Is the parent of the cell updated to the scope at the outer layer?

sudongyuer · 2022-12-15T04:30:08Z

@sudongyuer What will happen if you move the scope after the code cell has been moved out of it? Is the parent of the cell updated to the scope at the outer layer?

The node's parent will not change , it will remain the relationship before

Screen.Recording.2022-12-15.at.12.28.48.mov

sudongyuer · 2022-12-15T04:32:00Z

You can see more contexts in #99

forrestbao · 2022-12-15T05:30:06Z

@sudongyuer yes, i saw the discussion in #99. Should we update the parent then? Otherwise, the code scoping is not correct.

sudongyuer · 2022-12-15T08:24:41Z

@sudongyuer yes, i saw the discussion in #99. Should we update the parent then? Otherwise, the code scoping is not correct.

I just think this pr is to control the extent option in node , if you want to remove node's parent relationship , should be better add another feature to support that . Also need some suggestions from @lihebi

lihebi · 2022-12-15T15:37:02Z

Controlling only the "extent" option itself doesn't make a useful feature. We should implement the logic for updating node parent relations in this PR.

sudongyuer · 2022-12-16T05:22:03Z

Screen.Recording.2022-12-16.at.13.20.56.mov

li-xin-yi · 2022-12-16T07:41:01Z

Thanks for the great PR!

Please take care when updating the parent, you should update nodesMap[id].level recursively as well. My previous PR for reference: #145

forrestbao · 2022-12-16T08:11:30Z

Is recursive updating necessary? After a user clicks the "remove extent" button (the name needs to be changed later into something like "change scope") of a cell/scope, the cell/scope will move with the mouse. The user can move the mouse to move the cell/scope to where s/he sees fit and drop it there by clicking the mouse again. Just detect the node on which the mouse click happens and let it be the parent of the cell/scope just moved.

sudongyuer · 2022-12-16T08:25:34Z

Is recursive updating necessary? After a user clicks the "remove extent" button (the name needs to be changed later into something like "change scope") of a cell/scope, the cell/scope will move with the mouse. The user can move the mouse to move the cell/scope to where s/he sees fit and drop it there by clicking the mouse again. Just detect the node on which the mouse click happens and let it be the parent of the cell/scope just moved.

Now when click the remove from scope button , it will just remove the parent relationship , it's not save the relationship before , so when you click again , it will not restore the previous parent-son state , if you want to restore the previous state, you just need drag it into scope again , so recursive update is necessary now.

li-xin-yi · 2022-12-16T08:36:21Z

Is recursive updating necessary? After a user clicks the "remove extent" button (the name needs to be changed later into something like "change scope") of a cell/scope, the cell/scope will move with the mouse. The user can move the mouse to move the cell/scope to where s/he sees fit and drop it there by clicking the mouse again. Just detect the node on which the mouse click happens and let it be the parent of the cell/scope just moved.

It's always necessary to update the levels of all involved nodes when any scope relationship changes. The level determines the pod color in the mini-map, we need to show everything in the mini-map correctly in time.

The idea to drag and release sounds cool. But I want to make sure about something related to my implementation: what will a collaborator see during the process of changing scope? the complete movement track or nothing until it finishes?

lihebi · 2022-12-19T19:34:42Z

Screen.Recording.2022-12-16.at.13.20.56.mov

Thanks!

When you click the release button, the pod "jumps" out. It is because you updated its parent but didn't update its position (which is relative to the parent). We don't want that jump.

lihebi · 2022-12-19T19:47:46Z

[xinyi] you should update nodesMap[id].level recursively as well
[forrest] Is recursive updating necessary?
[xinyi] It's always necessary to update the levels

Xinyi had explained it. Just an echo: Xinyi was talking about recursively updating the "levels" property of descendant nodes, which is missing in this PR.

[forrest] After a user clicks the "remove extent" button of a cell/scope, the cell/scope will move with the mouse.

To make things simple, we could just drop the node to ROOT scope when the release button is clicked. We probably want to show some indicators (e.g., a highlighted shadow) to mark it global. Then we can drag-n-drop to the location.

[xinyi] what will a collaborator see during the process of changing scope?

This will no longer be a problem if we adopt the "drop-to-ROOT-scope" approach above.

ui/src/components/Canvas.tsx

sudongyuer · 2022-12-24T14:25:06Z

Thx xinyi ~

feat: pod dissociate from scope

b2c880c

sudongyuer marked this pull request as draft December 13, 2022 15:34

lihebi reviewed Dec 13, 2022

View reviewed changes

lihebi and others added 8 commits December 14, 2022 22:02

update README

29d78b2

add website link to README

366affa

fix: add a dfs function to update node levels (codepod-io#145)

5b50e78

use relation for collaborators (codepod-io#146)

33153f4

add hasToken to avoid loading repo to early (codepod-io#147)

e9528be

do not set repo.name on creation, use "Untitled" as placeholder (code…

76c59a0

…pod-io#148)

feat: support toggle extent

88edf5b

Merge branch 'codepod-io:main' into feature/dissociate_pod_from_scope

25b8525

sudongyuer marked this pull request as ready for review December 14, 2022 14:04

sudongyuer changed the title ~~feat: pod dissociate from scope~~ feat: support toggle extent Dec 14, 2022

chore: update

0adb115

sudongyuer mentioned this pull request Dec 14, 2022

Cannot move pod out of a scope #99

Closed

sudongyuer added 2 commits December 15, 2022 10:00

chore: update

56874bf

chore: reset node

69060c1

feat: support remove scope

2dd8085

sudongyuer requested a review from lihebi December 16, 2022 05:22

sudongyuer changed the title ~~feat: support toggle extent~~ feat: support remove scope Dec 16, 2022

chore: update level

3ba1663

chore: clean code

f2a9cfe

sudongyuer closed this Dec 20, 2022

sudongyuer reopened this Dec 20, 2022

sudongyuer commented Dec 20, 2022

View reviewed changes

ui/src/components/Canvas.tsx Show resolved Hide resolved

lihebi mentioned this pull request Dec 23, 2022

Feat: single-pod cut-n-paste support #165

Merged

sudongyuer closed this Dec 24, 2022

		{/<Controls showInteractive={role !== RoleType.GUEST} />/}

		{/<Background />/}

feat: support remove scope #143

feat: support remove scope #143

Uh oh!

Conversation

sudongyuer commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lihebi Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lihebi Dec 13, 2022

Choose a reason for hiding this comment

Uh oh!

sudongyuer Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

lihebi Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

lihebi Dec 13, 2022

Choose a reason for hiding this comment

Uh oh!

sudongyuer Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

lihebi Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

lihebi left a comment

Choose a reason for hiding this comment

Uh oh!

sudongyuer commented Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

forrestbao commented Dec 15, 2022

Uh oh!

sudongyuer commented Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sudongyuer commented Dec 15, 2022

Uh oh!

forrestbao commented Dec 15, 2022

Uh oh!

sudongyuer commented Dec 15, 2022

Uh oh!

lihebi commented Dec 15, 2022

Uh oh!

sudongyuer commented Dec 16, 2022

Uh oh!

li-xin-yi commented Dec 16, 2022

Uh oh!

forrestbao commented Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sudongyuer commented Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

li-xin-yi commented Dec 16, 2022

Uh oh!

lihebi commented Dec 19, 2022

Uh oh!

lihebi commented Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sudongyuer commented Dec 24, 2022

Uh oh!

Uh oh!

sudongyuer commented Dec 13, 2022 •

edited

Loading

lihebi Dec 13, 2022 •

edited

Loading

sudongyuer commented Dec 15, 2022 •

edited

Loading

sudongyuer commented Dec 15, 2022 •

edited

Loading

forrestbao commented Dec 16, 2022 •

edited

Loading

sudongyuer commented Dec 16, 2022 •

edited

Loading

lihebi commented Dec 19, 2022 •

edited

Loading