[2589] Improve the label component API to facilitate its customization #2590

frouene · 2023-11-17T13:11:46Z

Pull request template

General purpose

What is the main goal of this pull request?

Project management

Has the pull request been added to the relevant project and milestone? (Only if you know that your work is part of a specific iteration such as the current one)
Have the priority: and pr: labels been added to the pull request? (In case of doubt, start with the labels priority: low and pr: to review later)
Have the relevant issues been added to the pull request?
Have the relevant labels been added to the issues? (area:, difficulty:, type:)
Have the relevant issues been added to the same project and milestone as the pull request?
Has the CHANGELOG.adoc been updated to reference the relevant issues?
Have the relevant API breaks been described in the CHANGELOG.adoc? (Including changes in the GraphQL API)
In case of a change with a visual impact, are there any screenshots in the CHANGELOG.adoc? For example in doc/screenshots/2022.5.0-my-new-feature.png

Architectural decision records (ADR)

Does the title of the commit contributing the ADR start with [doc]?
Are the ADRs mentioned in the relevant section of the CHANGELOG.adoc?

Dependencies

Are the new / upgraded dependencies mentioned in the relevant section of the CHANGELOG.adoc?
Are the new dependencies justified in the CHANGELOG.adoc?

Frontend

This section is not relevant if your contribution does not come with changes to the frontend.

General purpose

Is the code properly tested? (Plain old JavaScript tests for business code and tests based on React Testing Library for the components)

Typing

We need to improve the typing of our code, as such, we require every contribution to come with proper TypeScript typing for both changes contributing new files and those modifying existing files.
Please ensure that the following statements are true for each file created or modified (this may require you to improve code outside of your contribution).

Backend

This section is not relevant if your contribution does not come with changes to the backend.

General purpose

Are all the event handlers tested?
Are the event processor tested?
Is the business code (services) tested?
Are diagram layout changes tested?

Architecture

Are data structure classes properly separated from behavioral classes?
Are all the relevant fields final?
Is any data structure mutable? If so, please write a comment indicating why
Are behavioral classes either stateless or side effect free?

Review

How to test this PR?

Please describe here the various use cases to test this pull request

Has the Kiwi TCMS test suite been updated with tests for this contribution?

Bug: #2589 Signed-off-by: Florian ROUËNÉ <florian.rouene@obeosoft.com>

sbegaudeau · 2023-11-20T19:54:15Z

...s/diagrams/frontend/sirius-components-diagrams-reactflow/src/renderer/node/IconLabelNode.tsx

+          <Label diagramElementId={id} label={data.label}>
+            <div style={labelStyle(data.faded, !!data.label.iconURL)}>
+              <LabelIcon />
+              <LabelText />
+            </div>
+          </Label>


Why are you changing the behavior of the label here. We were using the exact same Label component as in other nodes like ImageNode, no need to change that as part of this PR.

sbegaudeau · 2023-11-20T19:54:58Z

packages/core/frontend/sirius-components-core/src/icon/IconOverlay.tsx

+    minWidth: ({ iconWidth }) => `${iconWidth}px`,
+    minHeight: ({ iconHeight }) => `${iconHeight}px`,


This has nothing to do with this PR

sbegaudeau · 2023-11-20T19:56:49Z

...diagrams/frontend/sirius-components-diagrams-reactflow/src/renderer/DiagramRenderer.types.ts

@@ -82,6 +82,7 @@ export interface Label {
  text: string;
  iconURL: string[];
  style: React.CSSProperties;
+  borderStyle?: React.CSSProperties;


The properties of the border can be defined in style why do you need a new property for that?

sbegaudeau · 2023-11-20T19:59:09Z

.../diagrams/frontend/sirius-components-diagrams-reactflow/src/renderer/edge/MultiLabelEdge.tsx

          )}
          {label && (
-            <Label diagramElementId={id} transform={`translate(${labelX}px,${labelY}px)`} label={label} faded={false} />
+            <div style={{ transform: `translate(${labelX}px,${labelY}px)` }}>
+              <DiagramLabel diagramElementId={id} label={label} faded={false} />


This label should probably be faded when it's relevant just like the other labels

sbegaudeau · 2023-11-20T20:03:03Z

...s/diagrams/frontend/sirius-components-diagrams-reactflow/src/renderer/label/DiagramLabel.tsx

+    display: 'flex',
+    flexDirection: 'row',
+    alignItems: 'center',
+    justifyContent: borderStyle?.justifyContent ?? 'flex-start',


If someone needs the label at the start of some div, they can just move the whole DiagramLabel component. This option is not necessary.

sbegaudeau · 2023-11-20T20:05:25Z

...s/diagrams/frontend/sirius-components-diagrams-reactflow/src/renderer/label/DiagramLabel.tsx

+export const DiagramLabel = ({ diagramElementId, label, faded }: DiagramLabelProps) => {
+  return (
+    <Label diagramElementId={diagramElementId} label={label}>
+      <div style={labelStyle(faded, !!label.iconURL, label.borderStyle)}>


I don't think we need to consider the style of the label object, that's not relevant in the props of our component. If someone needs a border, they can add it the div containing DiagramLabel. Because tomorrow, someone else will come and ask for the customization of the CSS of the borderTop, or borderLeft or anything else. Just like transform was an issue related specifically to MultiLabelEdge, we have no need for a border or flex-start here.

sbegaudeau · 2023-11-20T20:06:11Z

packages/diagrams/frontend/sirius-components-diagrams-reactflow/src/renderer/label/Label.tsx

+  };
+
+  if (label.id === currentlyEditedLabelId) {
+    return <DiagramDirectEditInput editingKey={editingKey} onClose={handleClose} labelId={label.id} transform={''} />;


Is that transform still needed?

sbegaudeau · 2023-11-20T20:09:49Z

...es/diagrams/frontend/sirius-components-diagrams-reactflow/src/renderer/label/LabelContext.ts

+import React from 'react';
+import { Label } from '../DiagramRenderer.types';
+
+export const LabelContext = React.createContext<Label | undefined>(undefined);


Just put an empty Label here and remove the undefined it will simplify tons of code below that should never be used if the label is not there. In case you need to explicitly handle the lack of value, prefer null next time to help distinguish values which do not exist.

frouene linked an issue Nov 17, 2023 that may be closed by this pull request

Add customization possibilities for node labels #2589

Open

frouene force-pushed the fro/fix/label_api branch 3 times, most recently from 29d1f42 to 23a83d6 Compare November 20, 2023 09:40

frouene changed the title ~~[doc] Contribute an ADR to improve the label component API~~ [2589] Improve the label component API to facilitate its customization Nov 20, 2023

frouene added pr: to review later priority: low labels Nov 20, 2023

frouene added this to the 2023.12.0 milestone Nov 20, 2023

frouene force-pushed the fro/fix/label_api branch from 23a83d6 to 1d1348b Compare November 20, 2023 09:46

[2589] Improve the label component API to facilitate its customization

98726c8

Bug: #2589 Signed-off-by: Florian ROUËNÉ <florian.rouene@obeosoft.com>

frouene force-pushed the fro/fix/label_api branch from 1d1348b to 98726c8 Compare November 20, 2023 13:11

sbegaudeau requested changes Nov 20, 2023

View reviewed changes

sbegaudeau modified the milestones: 2023.12.0, 2024.1.0 Nov 23, 2023

sbegaudeau removed this from the 2024.1.0 milestone Dec 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2589] Improve the label component API to facilitate its customization #2590

[2589] Improve the label component API to facilitate its customization #2590

frouene commented Nov 17, 2023

sbegaudeau Nov 20, 2023

sbegaudeau Nov 20, 2023

sbegaudeau Nov 20, 2023

sbegaudeau Nov 20, 2023

sbegaudeau Nov 20, 2023

sbegaudeau Nov 20, 2023

sbegaudeau Nov 20, 2023

sbegaudeau Nov 20, 2023

		minWidth: ({ iconWidth }) => `${iconWidth}px`,
		minHeight: ({ iconHeight }) => `${iconHeight}px`,

[2589] Improve the label component API to facilitate its customization #2590

Are you sure you want to change the base?

[2589] Improve the label component API to facilitate its customization #2590

Conversation

frouene commented Nov 17, 2023

Pull request template

General purpose

Project management

Architectural decision records (ADR)

Dependencies

Frontend

General purpose

Typing

Backend

General purpose

Architecture

Review

How to test this PR?

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