[1988] Add support for optional help text on widgets #2011

pcdavid · 2023-05-31T15:52:37Z

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?

frouene · 2023-06-09T08:46:08Z

The helpExpression doesn't work for slider, I think the helpTextProvider is missing from the builder of Slider.java.

frouene · 2023-06-09T08:47:55Z

The helpExpression is not display for toolbarAction widget, but it seems to be the same behavior as for the label.

...nts-forms/src/main/java/org/eclipse/sirius/components/forms/renderer/FormElementFactory.java

.../org/eclipse/sirius/components/collaborative/forms/handlers/HelpTextRequestEventHandler.java

...ackend/sirius-components-forms/src/main/java/org/eclipse/sirius/components/forms/Select.java

sbegaudeau

I have not tested it but what is that git all file exactly?

...se/sirius/components/forms/graphql/datafetchers/form/FormDescriptionHelpTextDataFetcher.java

git all

sbegaudeau · 2023-06-09T09:34:25Z

...ackend/sirius-components-forms/src/main/java/org/eclipse/sirius/components/forms/Button.java

@@ -96,6 +96,8 @@ public static final class Builder {

        private List<Diagnostic> diagnostics;

+        private Supplier<String> helpTextProvider;


I suspect this is what you were talking about with the modification of dozens of files with widgets, components, descriptions to set that up. In the diagram representation, we went the other way by removing the description as the sole mean to carry information for a service and instead try to use its content only for the rendering.

You could have introduced something like that to ask for the help text and use it in the handler:

public interface IWidgetHelpTextProvider { String getHelpText(...) }

When the frontend would ask for the help text, you would just ask this service which could then leverage the view model to find the relevant information. Here, the converters needs to setup everything first. Which is why you have to make the help text provider optional because it needs to carry two pieces of informations at once:

is there some help text? (for the rendering)

what is the value of the help text?

You could have simply used a boolean to carry the information relevant for the rendering.

We could also introduce a common AbstractWidgetBuilder which would be inherited by all the Builders of the widgets. It could encapsulate things like the id, presence of help text, diagnostics, label. I don't see any issues with having some simple inheritance for builders like that since builders are 99% data structure.

You could have simply used a boolean to carry the information relevant for the rendering.

Yes, but I would still have needed about the same amount of changes to carry the boolean (instead of the Supplier<String>), wouldn't I?

I don't see any issues with having some simple inheritance for builders like that since builders are 99% data structure.

AFAICT, it's forbidden by the rules you setup: immutableClassesShouldHaveANestedBuilder, and HaveAValidBuilderCondition requires the builder class to be final.

When the frontend would ask for the help text, you would just ask this service which could then leverage the view model

What about widgets created programatically which do not come from a View model?

pcdavid · 2023-06-09T14:10:20Z

The helpExpression doesn't work for slider, I think the helpTextProvider is missing from the builder of Slider.java.

There was a little more missing, but done.

The Git all file has been removed :)

pcdavid · 2023-06-12T09:47:05Z

The helpExpression is not display for toolbarAction widget, but it seems to be the same behavior as for the label.

I've added help support for toolbarActions. The tooltip is associated to the button itself instead of a separate "?" icon (there is not really any room for such an icon in this case).

Bug: #1988 Signed-off-by: Pierre-Charles David <pierre-charles.david@obeo.fr>

Signed-off-by: Stéphane Bégaudeau <stephane.begaudeau@obeo.fr>

sbegaudeau · 2023-06-19T14:00:25Z

packages/forms/frontend/sirius-components-forms/src/propertysections/HelpTooltip.types.ts

+  editingContextId: string;
+  formId: string;
+  widgetId: string;
+  children?: any;


React.ReactElement would work here

sbegaudeau · 2023-06-19T14:00:52Z

packages/forms/frontend/sirius-components-forms/src/propertysections/HelpTooltip.tsx

+  const [open, setOpen] = useState(false);
+  const [content, setContent] = useState<JSX.Element>(null);


You could consolidate both state into one.

sbegaudeau · 2023-06-19T14:01:19Z

packages/forms/frontend/sirius-components-forms/src/propertysections/HelpTooltip.tsx

+              {lines.map((line) => (
+                <>
+                  {line}
+                  <br />
+                </>
+              ))}


Use React.Fragment to have access to the key props to remove the warning

sbegaudeau · 2023-06-19T14:04:11Z

packages/forms/frontend/sirius-components-forms/src/propertysections/HelpTooltip.tsx

+    <Tooltip
+      open={open && content !== null}
+      onClose={handleClose}
+      onOpen={handleOpen}
+      title={content || ''}
+      arrow
+      placement={'top'}>
+      {props.children || <HelpOutlineOutlined color="secondary" style={{ marginLeft: 8, fontSize: 16 }} />}
+    </Tooltip>
+  );


I'm still not sure why but moving the cursor a tiny bit can trigger a lot of HTTP request to reload the tooltip. I wonder if there is some options in MUI which would prevent so many open/close events

pcdavid changed the title ~~Add support for optional help text on widgets~~ [1988] Add support for optional help text on widgets May 31, 2023

pcdavid added pr: to review later priority: low labels May 31, 2023

pcdavid added this to the 2023.8.0 milestone May 31, 2023

pcdavid linked an issue May 31, 2023 that may be closed by this pull request

Add support for optional help text on widgets #1988

Closed

11 tasks

pcdavid force-pushed the pcd/enh/widget-help branch 3 times, most recently from 351349f to 12128fb Compare June 5, 2023 08:00

pcdavid marked this pull request as ready for review June 5, 2023 08:01

pcdavid requested review from sbegaudeau and AxelRICHARD as code owners June 5, 2023 08:01

pcdavid force-pushed the pcd/enh/widget-help branch 9 times, most recently from 281a777 to cb34647 Compare June 7, 2023 06:53

frouene requested changes Jun 9, 2023

View reviewed changes

sbegaudeau previously requested changes Jun 9, 2023

View reviewed changes

pcdavid force-pushed the pcd/enh/widget-help branch 2 times, most recently from 42bf953 to e21eda7 Compare June 12, 2023 09:45

pcdavid force-pushed the pcd/enh/widget-help branch from e21eda7 to d33c1db Compare June 15, 2023 09:00

pcdavid force-pushed the pcd/enh/widget-help branch from d33c1db to 78b2bb5 Compare June 15, 2023 09:17

pcdavid added pr: to review soon priority: high and removed pr: to review later priority: low labels Jun 15, 2023

pcdavid requested a review from sbegaudeau June 15, 2023 09:18

pcdavid force-pushed the pcd/enh/widget-help branch from 78b2bb5 to b26d9a3 Compare June 15, 2023 09:40

sbegaudeau force-pushed the pcd/enh/widget-help branch from b26d9a3 to 8ae7820 Compare June 19, 2023 12:04

pcdavid and others added 2 commits June 19, 2023 16:04

[1988] Add support for optional help text on widgets

cc74f97

Bug: #1988 Signed-off-by: Pierre-Charles David <pierre-charles.david@obeo.fr>

[releng] Bump version to 2023.6.1

2ec6907

Signed-off-by: Stéphane Bégaudeau <stephane.begaudeau@obeo.fr>

sbegaudeau force-pushed the pcd/enh/widget-help branch from 8ae7820 to 2ec6907 Compare June 19, 2023 14:06

sbegaudeau requested a review from gcoutable as a code owner June 19, 2023 14:06

sbegaudeau reviewed Jun 19, 2023

View reviewed changes

sbegaudeau approved these changes Jun 19, 2023

View reviewed changes

sbegaudeau merged commit 34a9725 into master Jun 19, 2023
4 checks passed

pcdavid deleted the pcd/enh/widget-help branch June 30, 2023 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1988] Add support for optional help text on widgets #2011

[1988] Add support for optional help text on widgets #2011

pcdavid commented May 31, 2023 •

edited

frouene commented Jun 9, 2023

frouene commented Jun 9, 2023

sbegaudeau left a comment

sbegaudeau Jun 9, 2023

pcdavid Jun 9, 2023

pcdavid Jun 9, 2023

pcdavid commented Jun 9, 2023

pcdavid commented Jun 12, 2023

sbegaudeau Jun 19, 2023

sbegaudeau Jun 19, 2023

sbegaudeau Jun 19, 2023

sbegaudeau Jun 19, 2023

		@@ -96,6 +96,8 @@ public static final class Builder {

		private List<Diagnostic> diagnostics;

		private Supplier<String> helpTextProvider;

		const [open, setOpen] = useState(false);
		const [content, setContent] = useState<JSX.Element>(null);

[1988] Add support for optional help text on widgets #2011

[1988] Add support for optional help text on widgets #2011

Conversation

pcdavid commented May 31, 2023 • edited

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?

frouene commented Jun 9, 2023

frouene commented Jun 9, 2023

sbegaudeau left a comment

Choose a reason for hiding this comment

sbegaudeau Jun 9, 2023

Choose a reason for hiding this comment

pcdavid Jun 9, 2023

Choose a reason for hiding this comment

pcdavid Jun 9, 2023

Choose a reason for hiding this comment

pcdavid commented Jun 9, 2023

pcdavid commented Jun 12, 2023

sbegaudeau Jun 19, 2023

Choose a reason for hiding this comment

sbegaudeau Jun 19, 2023

Choose a reason for hiding this comment

sbegaudeau Jun 19, 2023

Choose a reason for hiding this comment

sbegaudeau Jun 19, 2023

Choose a reason for hiding this comment

pcdavid commented May 31, 2023 •

edited