Issue/composer v2 #5985

matborowczyk · 2024-10-15T14:41:02Z

Description

Pull request that merge all changes made to the instance composer to move away from old design and functionalities and make composer to follow form-like behaviour

Task that are still required for full redesign:

#5870 #5986 #5987 #5988 #5989 #5990 #5991

Screen.Recording.2024-10-16.at.12.39.47.mov

closes #5868

Self Check:

Strike through any lines that are not applicable (~~line~~) then check the box

Attached issue to pull request
Changelog entry
Code is clear and sufficiently documented
Sufficient test cases (reproduces the bug/tests the requested feature)
Correct, in line with design
End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )

…73-persisting-coordinates

Co-authored-by: Lukas <[email protected]>

…73-persisting-coordinates

…a/web-console into issue/5273-persisting-coordinates

…73-persisting-coordinates

…stanceWithRelations.ts Co-authored-by: Lukas <[email protected]>

Co-authored-by: Lukas <[email protected]>

…to issue/composer-v2

LukasStordeur

I re-reviewed the first part of the PR.

changelogs/unreleased/5868-composer-v2.yml

LukasStordeur · 2024-10-17T09:15:27Z

src/Data/Managers/V2/GETTERS/GetInstanceWithRelations/useGetInstanceWithRelations.ts

+      serviceModel.inter_service_relations?.map((relation) => relation.name) ||
+      [];
+
+    const nestedRelations = serviceModel.embedded_entities?.flatMap((entity) =>


For relations, you are adding the safe returning of an empty array, but not here. Typescript might still infer the result of the flatmap as undefined.

Here I should remove "?" because according to interface embedded entities in service model cant be undefined, I checked on test env it seems to align

LukasStordeur · 2024-10-17T11:06:51Z

src/UI/Components/Diagram/Context/EventWrapper.tsx

+
+  /**
+   * Handles the event triggered when there are loose embedded entities on the canvas.
+   * The loose embedded entities are the entities that are not connected to the main entity. If there are any, they will be highlighted and deploy button will be disabled.


What with the Inter-service relations? Is the deploy disabled too when a loose Inter-service relation is dragged on the canvas?

no, I'll create ticket for that

LukasStordeur · 2024-10-17T11:07:33Z

src/UI/Components/Diagram/Context/EventWrapper.tsx

+    };
+
+    setServiceOrderItems((prev) => {
+      // related instances aren't added to the serviceOrderItems map, this condition is here to prevent situation where we try to remove the related instance from the canvas and it ends up here with status to delete it from the inventory


here too, be more explicit. what are the related instances?

I think it's quite explicit, especially that relation in app is I think exclusively related(no pun intended) to inter-service relation

Well, that's the thing, sometimes related is used for embedded or inter-service-relations. We have an explicit term, so we should use it to avoid any possible confusion. If we had no term for them, sure, but we have the right terms. There's no reason to re-invent them :)

Is it used though? I never came across that use but, maybe I missed something, I'll change it :)

LukasStordeur · 2024-10-17T11:08:24Z

src/UI/Components/Diagram/Context/EventWrapper.tsx

+  };
+
+  /**
+   * Handles updating the stencil state for the embedded entities.


The name of the method isn't mentionning the embedded entities, maybe make the method name more explicit.

I removed the embedded entities part ,as stencil state holds only those, so it's redundant here, added more into description of the state in the context docstrings

Did you push the changes? I don't see them.

There all should be there

src/UI/Components/Diagram/Context/EventWrapper.tsx

LukasStordeur · 2024-10-17T11:13:25Z

src/UI/Components/Diagram/Context/EventWrapper.tsx

+        stencil.max !== undefined &&
+        stencil.current >= stencil.max;
+
+      // If the current count of the instances created from given stencil is more than or equal to the max count, disable the stencil of given embedded entity


update based on feedback above.

In case of what I wrote above, there is nothing to update, right?

Is it then worth adding a comment that is simply written differently than the previous one, but actually would mean the same?

Co-authored-by: Lukas <[email protected]>

…stanceWithRelations.ts Co-authored-by: Lukas <[email protected]>

Co-authored-by: Lukas <[email protected]>

…to issue/composer-v2

LukasStordeur

I haven't gone through some of the tests yet, but I'll do that Tuesday

LukasStordeur · 2024-10-18T13:51:48Z

src/UI/Components/Diagram/Canvas.tsx


+  /**
+   * create the stencil sidebar only left sidebar ref, the scroller, related inventories by inter-service relations, main service and service models are defined


the comment make it feel like left sidebar is something else than the stencil sidebar, while they are in fact the same :)

LukasStordeur · 2024-10-18T13:53:04Z

src/UI/Components/Diagram/Canvas.tsx

-        </ZoomWrapper>
+        <StencilContainer
+          className="stencil-sidebar"
+          data-testid="left_sidebar"


I'd like it to be consistently called the same, either you opt for stencil-sidebar, or left-sidebar :)

LukasStordeur · 2024-10-18T14:01:33Z

src/UI/Components/Diagram/Context/ComposerEditorProvider.tsx

+      />
+    );
+  }
+  if (instanceWithRelationsQuery.isError) {


hmm, I see now what you meant in our call about this...
Perhaps this isn't the best solution either.
Maybe having the message/retry/ariaLabel in a React.state would make more sense, a bit like we do for the toaster in general? And have a useEffect on any of the errors which would update the state-variables. Just an idea for improvements, can eventually be a ticket to have a better look at how we do error handling in general for queries. When there are a few queries present on the same page, it sometimes becomes very repetitive.

LukasStordeur · 2024-10-18T14:03:22Z

src/UI/Components/Diagram/Context/EventWrapper.tsx

+        stencil.max !== undefined &&
+        stencil.current >= stencil.max;
+
+      // If the current count of the instances created from given stencil is more than or equal to the max count, disable the stencil of given embedded entity


Is it then worth adding a comment that is simply written differently than the previous one, but actually would mean the same?

LukasStordeur · 2024-10-18T14:05:15Z

src/UI/Components/Diagram/actions.ts

@@ -173,84 +160,210 @@ export function showLinkTools(
  linkView.addTools(tools);
 }

+/**
+ * Function that creates, appends and returns created Entity, differs from appendInstance by the fact that is used from the scope of Instance Composer and uses different set of data


Comment isn't exactly clear which one uses the scope of the instance composer. And does it use a different set of data, or different sets of date (isn't exactly the same)

LukasStordeur · 2024-10-18T14:57:19Z

src/UI/Components/Diagram/stencil/stencil.ts

+const PADDING_S = GRID_SIZE;
+
+/**
+ * Class initializing the Service Instance Stencil Tab.


I think the name should be more clearer, and it would be nice to specify what exactly this tab contains. In the designs it's called the New tab, maybe something with the new-keyword would make it clearer.

And since you're using classes, perhaps using inheritance in this very case could be interesting. I'm not that familiar with class based patterns. But I see some similarities between the two tabs

LukasStordeur · 2024-10-18T15:04:09Z

src/UI/Components/Diagram/stencil/stencil.ts

+/**
+ * Class representing a stencil sidebar.
+ */
+export class StencilSidebar {


I think it might be nice to have each class in a separate file and under a folder

LukasStordeur · 2024-10-18T15:06:02Z

src/UI/Components/Diagram/zoomHandler/zoomHandler.ts

+}
+
+/**
+ * IconButton class


Naming this a class while it's a const can be confusing.

LukasStordeur · 2024-10-18T15:07:53Z

src/UI/Components/Diagram/zoomHandler/zoomHandler.ts

+/**
+ * ZoomHandlerService class
+ *
+ * This class is responsible for managing the navigation of the canvas.


Paning would be a better word than navigation :) Navigation implies routing, or boats, or GPS

LukasStordeur · 2024-10-18T15:10:35Z

src/UI/Components/Diagram/zoomHandler/zoomHandler.ts

+      "fullscreen",
+    ) as IconButton;
+
+    if (document.fullscreenElement) {


This if else could be improved like the snippet I gave you where you needed to toggle the classes.

matborowczyk and others added 30 commits April 10, 2024 11:42

Add persisting coordinates locally

2379ccd

docstrings

6eeef14

docstrings

039b24e

docstrings

9ba9174

Merge branch 'master' of github.com:inmanta/web-console into issue/52…

6915af5

…73-persisting-coordinates

Update src/UI/Components/Diagram/Canvas.tsx

761e73e

Co-authored-by: Lukas <[email protected]>

Update src/UI/Components/Diagram/Canvas.tsx

6d877f4

Co-authored-by: Lukas <[email protected]>

Update src/UI/Components/Diagram/Canvas.tsx

d5ad192

Co-authored-by: Lukas <[email protected]>

add metadata implementation

0e1458f

Merge branch 'master' of github.com:inmanta/web-console into issue/52…

67a3e7c

…73-persisting-coordinates

Merge branch 'issue/5273-persisting-coordinates' of github.com:inmant…

1eff8aa

…a/web-console into issue/5273-persisting-coordinates

remove comment

aeaa546

fix, faulty version error

1f68667

Merge branch 'master' of github.com:inmanta/web-console into issue/52…

186649a

…73-persisting-coordinates

cypress suite fix

73b0c0a

Merge branch 'master' of github.com:inmanta/web-console into issue/52…

ca1a625

…73-persisting-coordinates

move coordinates data to order api call

6e94efe

remove casting

5bc03a4

cleanup

2016e84

e2e fix

b103d2d

fix test

094ddf1

clean up

6a47082

test fix

75369ff

remarks & improvements

f86f405

extend docstring

f853a4d

e2e fix

29f8888

remarks

69f994e

remarks

ea53c2d

typing for Canvas FC

787238a

e2e fix

8add30f

matborowczyk and others added 8 commits October 17, 2024 07:24

Update src/Data/Managers/V2/GETTERS/GetInstanceWithRelations/useGetIn…

68d1a7e

…stanceWithRelations.ts Co-authored-by: Lukas <[email protected]>

Update src/UI/Components/Diagram/Canvas.tsx

ab819fe

Co-authored-by: Lukas <[email protected]>

Update src/UI/Components/Diagram/Context/EventWrapper.tsx

fcb3205

Co-authored-by: Lukas <[email protected]>

Update src/UI/Components/Diagram/Context/CanvasProvider.tsx

fb3b105

Co-authored-by: Lukas <[email protected]>

Update src/UI/Components/Diagram/Context/EventWrapper.tsx

8eaf91f

Co-authored-by: Lukas <[email protected]>

remarks

2f17d16

Merge branch 'issue/composer-v2' of github.com:inmanta/web-console in…

bf5240d

…to issue/composer-v2

tsc fix

26ddedb

LukasStordeur requested changes Oct 17, 2024

View reviewed changes

matborowczyk and others added 6 commits October 18, 2024 09:29

Update changelogs/unreleased/5868-composer-v2.yml

0ec04d9

Co-authored-by: Lukas <[email protected]>

Update changelogs/unreleased/5868-composer-v2.yml

f14fbfe

Co-authored-by: Lukas <[email protected]>

Update changelogs/unreleased/5868-composer-v2.yml

d6b07c3

Co-authored-by: Lukas <[email protected]>

Update changelogs/unreleased/5868-composer-v2.yml

ba78adc

Co-authored-by: Lukas <[email protected]>

Update src/UI/Components/Diagram/Context/Context.tsx

2368671

Co-authored-by: Lukas <[email protected]>

Update src/UI/Components/Diagram/Context/EventWrapper.tsx

aa6020d

Co-authored-by: Lukas <[email protected]>

matborowczyk mentioned this pull request Oct 18, 2024

Improve UseEffect in components #5997

Open

matborowczyk and others added 9 commits October 18, 2024 11:20

Update src/UI/Components/Diagram/Context/CanvasProvider.tsx

b376e97

Co-authored-by: Lukas <[email protected]>

Update src/Data/Managers/V2/GETTERS/GetInstanceWithRelations/useGetIn…

badffa9

…stanceWithRelations.ts Co-authored-by: Lukas <[email protected]>

Update src/UI/Components/Diagram/Context/ComposerCreatorProvider.tsx

8070b60

Co-authored-by: Lukas <[email protected]>

remarks

ea8ad8d

Merge branch 'issue/composer-v2' of github.com:inmanta/web-console in…

090c442

…to issue/composer-v2

remark

5dba6cf

remarks

19f5c33

remarks

6c5c878

further specify stencil state

218eebf

matborowczyk requested a review from LukasStordeur October 18, 2024 12:27

matborowczyk added 3 commits October 18, 2024 14:53

relation remarks

6f54361

relation remark

cace0d3

errorView remark

8d0c7b6

LukasStordeur reviewed Oct 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/composer v2 #5985

Issue/composer v2 #5985

matborowczyk commented Oct 15, 2024 •

edited

Loading

LukasStordeur left a comment

LukasStordeur Oct 17, 2024

matborowczyk Oct 18, 2024

LukasStordeur Oct 17, 2024

matborowczyk Oct 18, 2024

LukasStordeur Oct 17, 2024

matborowczyk Oct 18, 2024

LukasStordeur Oct 18, 2024 •

edited

Loading

matborowczyk Oct 18, 2024

LukasStordeur Oct 17, 2024

matborowczyk Oct 18, 2024

LukasStordeur Oct 18, 2024

matborowczyk Oct 18, 2024

LukasStordeur Oct 17, 2024

matborowczyk Oct 18, 2024

LukasStordeur Oct 18, 2024

LukasStordeur left a comment

LukasStordeur Oct 18, 2024

LukasStordeur Oct 18, 2024

LukasStordeur Oct 18, 2024

LukasStordeur Oct 18, 2024

LukasStordeur Oct 18, 2024

LukasStordeur Oct 18, 2024

LukasStordeur Oct 18, 2024

LukasStordeur Oct 18, 2024

LukasStordeur Oct 18, 2024

LukasStordeur Oct 18, 2024


		/**
		* create the stencil sidebar only left sidebar ref, the scroller, related inventories by inter-service relations, main service and service models are defined

Issue/composer v2 #5985

Are you sure you want to change the base?

Issue/composer v2 #5985

Conversation

matborowczyk commented Oct 15, 2024 • edited Loading

Description

Self Check:

LukasStordeur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasStordeur Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasStordeur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matborowczyk commented Oct 15, 2024 •

edited

Loading

LukasStordeur Oct 18, 2024 •

edited

Loading