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

fix(web): templates store - update the workflow editor position when selecting the blueprint template #3487

Merged
merged 3 commits into from
May 24, 2023

Conversation

LetItRock
Copy link
Contributor

What change does this PR introduce?

In this PR:

  • update the Workflow Editor position when selecting the template from the Templates Store modal
  • fixed the issue with wrong-positioned tree nodes when the new node is added (check screenshot)

Why was this change needed?

Templates Store PRD.

Other information (Screenshots)

  1. Wrong-positioned nodes
    Screenshot 2023-05-23 at 20 31 19

  2. Fixes

Screen.Recording.2023-05-23.at.20.29.10.mov
Screen.Recording.2023-05-23.at.20.29.42.mov

@LetItRock LetItRock self-assigned this May 23, 2023
@linear
Copy link

linear bot commented May 23, 2023

NV-2376 🏪 [Template store] Update Workflow Editor position

Reproduction Steps

Right now if you select template and scroll the Workflow Editor and click on any other blueprint template, then the Workflow Editor position is preserved, we want to update it and re-render basically on the tree from the top-centered, like it is initialised in the beginning.

Expected Behaviour

Update the Workflow Editor position when clicking on the blueprint template in the Templates Store modal.

Comment on lines +87 to +88
const [nodes, setNodes, onNodesChange] = useNodesState([]);
const [edges, setEdges, onEdgesChange] = useEdgesState<IEdge>([]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

by default we don't set the initial nodes and edges

reactFlowInstance?.project({ x: middle, y: 10 });
setViewport({ x: middle, y: 10, zoom: zoomView });

reactFlowInstance.setViewport({ x: middle, y: 10, zoom: zoomView });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

then we update the viewport to be top-centered

Comment on lines +102 to +104
setTimeout(() => {
initializeWorkflowTree();
}, 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After the viewport is updated we render the nodes and edges. This way we are skipping the initial blink that can be noticed here:

Screen.Recording.2023-05-23.at.20.38.33.mov


if (steps.length) {
for (let i = 0; i < steps.length; i++) {
const step = steps[i];
const oldNode = nodes[i + 1] || { position: { x: 0, y: 120 } };
const oldNode = nodes[i + 1];
const position = oldNode && oldNode.type !== 'addNode' ? oldNode.position : { x: 0, y: 120 };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed the nodes positioning issue

Comment on lines +183 to +184
setNodes(finalNodes);
setEdges(finalEdges);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

set all the nodes and edges at the end

@@ -106,6 +106,7 @@ export const TemplatesStoreModal = ({ general, isOpened, onClose }: ITemplatesSt
<CanvasHolder>
<ReactFlowProvider>
<FlowEditor
key={selectedTemplate._id}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this way we can update the Workflow Editor position when another blueprint template is selected

Copy link
Contributor

Choose a reason for hiding this comment

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

By the react rerender?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the selectedTemplate._id has changed, the rerender aka reconciliation algorithm will unmount FlowEditor and mount it again because the key prop has changed.
Basically, when React does rerender the same element in the tree it checks if the type is the same, or the key is the same, if any of these changes it does unmount for the old component, and mounts for the new one.

Copy link
Contributor

@djabarovgeorge djabarovgeorge 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 left a couple of comments, on the things I was not sure about.

const newId = (step._id || step.id) as string;

await onStepInit?.(step);

const newNode = buildNewNode(newId, oldNode, parentId, step, i);
setNodes((nds) => nds.concat(newNode));
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why there was a concat 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because before at the beginning of this function call initializeWorkflowTree we were resetting the nodes state to [trigger, addNode]. And I guess it only worked because we have the await in the line 168, the React setState is async.

initializeWorkflowTree();
setTimeout(() => {
initializeWorkflowTree();
}, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a setTimeout of 0? did you face race or something else while development?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The set timeout is like saying after the viewport is updated. We have two useEffects, the first one is adjusting the viewport to top-center, and in the second we are saying with the timeout: "when the viewport is updated, then build all the nodes".
The setTimeout(callback, 0) Web API puts the callback in the callback queue, which is executed right away when the JS callstack is empty (like when JS/React finishes it's job), so at that time, we are sure that the viewport is updated.


if (steps.length) {
for (let i = 0; i < steps.length; i++) {
const step = steps[i];
const oldNode = nodes[i + 1] || { position: { x: 0, y: 120 } };
const oldNode = nodes[i + 1];
const position = oldNode && oldNode.type !== 'addNode' ? oldNode.position : { x: 0, y: 120 };
Copy link
Contributor

Choose a reason for hiding this comment

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

You remember where is { x: 0, y: 120 } pointing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

from what I do remember it says to put the new node 120px below the old one

@@ -106,6 +106,7 @@ export const TemplatesStoreModal = ({ general, isOpened, onClose }: ITemplatesSt
<CanvasHolder>
<ReactFlowProvider>
<FlowEditor
key={selectedTemplate._id}
Copy link
Contributor

Choose a reason for hiding this comment

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

By the react rerender?

Base automatically changed from nv-2234-template-store-hover-on-popular to next May 24, 2023 12:52
@LetItRock LetItRock added this pull request to the merge queue May 24, 2023
Merged via the queue into next with commit d4ee8f6 May 24, 2023
25 checks passed
@LetItRock LetItRock deleted the nv-2376-update-workflow-editor-position branch May 24, 2023 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants