-
Notifications
You must be signed in to change notification settings - Fork 50
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
[1912] Add the possibility to send feedback messages to the frontend … #1978
[1912] Add the possibility to send feedback messages to the frontend … #1978
Conversation
7bd277f
to
6bee007
Compare
6bee007
to
09c89a3
Compare
packages/core/frontend/sirius-components-core/src/workbench/Toast.tsx
Outdated
Show resolved
Hide resolved
...nts-web/src/main/java/org/eclipse/sirius/components/web/services/FeedbackMessageService.java
Outdated
Show resolved
Hide resolved
private String applyLevelEmoji(FeedbackMessage feedback) { | ||
String prefix = switch (feedback.level) { | ||
case DEBUG -> new String(Character.toChars(0x1F41B)); | ||
case INFO -> new String(Character.toChars(0x2139)); | ||
case WARNING -> new String(Character.toChars(0x26A0)); | ||
case ERROR -> new String(Character.toChars(0x274C)); | ||
}; | ||
return String.format("%s %s", prefix, feedback.message); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really mind but given the profile of our customers we may not end up keeping emojis in the user interface and doing that on the frontend would probably be better since it would allow us to customize the appearance of the toast (its color for example) but that's a subject for later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea what these emojis are given just their code point :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that for the moment we don't have in the frontend the level of the return message since we only pass the text of the message.
I can change by suffixing with the level, for example [WARNING] the warning message
At first, we will join all the _string messages_ stacked in a single string, and append it to the existing `Failure` (`IStatus` implementation) _message_ parameter. | ||
In doing so, we can keep the actual _frontend_ representation for this message. | ||
|
||
To differentiate the level applied to the message, we will prefix it with an emoji representing this level. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
emoji or icon?
...tioneditors/frontend/sirius-components-formdescriptioneditors/src/FlexboxContainerWidget.tsx
Show resolved
Hide resolved
packages/core/frontend/sirius-components-core/src/workbench/Toast.tsx
Outdated
Show resolved
Hide resolved
packages/validation/frontend/sirius-components-validation/src/ValidationView.tsx
Outdated
Show resolved
Hide resolved
private String applyLevelEmoji(FeedbackMessage feedback) { | ||
String prefix = switch (feedback.level) { | ||
case DEBUG -> new String(Character.toChars(0x1F41B)); | ||
case INFO -> new String(Character.toChars(0x2139)); | ||
case WARNING -> new String(Character.toChars(0x26A0)); | ||
case ERROR -> new String(Character.toChars(0x274C)); | ||
}; | ||
return String.format("%s %s", prefix, feedback.message); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea what these emojis are given just their code point :-)
} | ||
|
||
public IStatus executeTool(Tool tool, VariableManager variableManager) { | ||
Optional<VariableManager> optionalVariableManager = this.executeOperations(tool.getBody(), variableManager); | ||
if (optionalVariableManager.isEmpty()) { | ||
return new Failure(String.format("Something went wrong while executing the tool '%s'", tool.getName())); | ||
var errorMessages = new ArrayList<>(List.of(String.format("Something went wrong while executing the tool '%s'", tool.getName()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errorMessages
=> feedbackMessages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that if the tool sends an "INFO"-level message it will get prefixed by "Something went wrong while executing the tool"?
If yes, this seems wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this first implementation we don't want to change the frontend behaviour, so we can only add stack messages on an error return.
But in the final version, if we imagine an action with several called services and before an error some information messages are stacked, then these messages will be displayed.
packages/core/frontend/sirius-components-core/src/workbench/Toast.tsx
Outdated
Show resolved
Hide resolved
...rc/main/java/org/eclipse/sirius/components/view/emf/diagram/DiagramOperationInterpreter.java
Outdated
Show resolved
Hide resolved
...nents-core/src/main/java/org/eclipse/sirius/components/core/api/IFeedbackMessageService.java
Show resolved
Hide resolved
6efd314
to
614e13e
Compare
I take it from here. |
f5a6b79
to
398320d
Compare
Bug: eclipse-sirius#1912 Signed-off-by: Florian Rouëné <florian.rouene@obeosoft.com>
398320d
to
a809ed1
Compare
…after an action
Bug: #1912
Pull request template
General purpose
What is the main goal of this pull request?
Project management
priority:
andpr:
labels been added to the pull request? (In case of doubt, start with the labelspriority: low
andpr: to review later
)area:
,difficulty:
,type:
)CHANGELOG.adoc
been updated to reference the relevant issues?CHANGELOG.adoc
? (Including changes in the GraphQL API)CHANGELOG.adoc
? For example indoc/screenshots/2022.5.0-my-new-feature.png
Architectural decision records (ADR)
[doc]
?CHANGELOG.adoc
?Dependencies
CHANGELOG.adoc
?CHANGELOG.adoc
?Frontend
This section is not relevant if your contribution does not come with changes to the frontend.
General purpose
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).
useMutation<DATA_TYPE, VARIABLE_TYPE>(…)
useQuery<DATA_TYPE, VARIABLE_TYPE>(…)
useSubscription<DATA_TYPE, VARIABLE_TYPE>(…)
useMachine<CONTEXT_TYPE, EVENTS_TYPE>(…)
useState<STATE_TYPE>(…)
?.
(if the GraphQL API specifies that a field cannot benull
, do not treat it has potentiallynull
for example)let diagram: Diagram | null = null;
)Backend
This section is not relevant if your contribution does not come with changes to the backend.
General purpose
Architecture
Review
How to test this PR?
Please describe here the various use cases to test this pull request