[2325] Add default and initial options to expressions popup#2337
[2325] Add default and initial options to expressions popup#2337AxelRICHARD wants to merge 3 commits into
Conversation
| * @param supportsFeatureValueProperties | ||
| * whether the effective expression is owned by a {@code FeatureValue}. | ||
| * @param featureValueProperties | ||
| * the current feature value properties when supported. |
There was a problem hiding this comment.
Given that the featureValueProperties is optional/nullable, is there a reason to have an explicit separate flag to indicate if it's supported?
| editingContextId: string, | ||
| elementId: string, | ||
| newExpressionText: string, | ||
| properties?: GQLEditExpressionInput['properties'] |
There was a problem hiding this comment.
Here the type is properties?: GQLEditExpressionInput['properties'], but in useEditExpression.types.ts it is defined as properties?: ExpressionProperties | null. They're probably equivalent, but why two different forms?
| editingContextId: string, | ||
| parentElementId: string, | ||
| expressionText: string, | ||
| properties?: GQLCreateExpressionInput['properties'] |
There was a problem hiding this comment.
Here the type is properties?: GQLCreateExpressionInput['properties'], but in useCrateExpression.types.tsit is defined asproperties?: ExpressionProperties | null`. They're probably equivalent, but why two different forms?
There was a problem hiding this comment.
Nitpick: is it voluntary to use a lower-case 'e' here? Except for hook types files, all other types.ts start with an upper-case letter.
| elementId, | ||
| mode, | ||
| onClose, | ||
| expressionPropertiesContext, |
There was a problem hiding this comment.
I'm probably missing something, but I'm not sure I get the purpose of this parameter. It's called "context" but is not a React context, and I don't seem to find any place where EditSysMLExpressionModal is invoked and it is actually passed (e.g. NewExpressionDiagramToolOverriddenContribution).
| featureValue: FeatureValueExpressionProperties; | ||
| } | ||
|
|
||
| export interface ExpressionPropertiesContext { |
There was a problem hiding this comment.
Strange to call this a "context" as it does not seem to have anything to do with React (use)Context.
2 participants
PLEASE READ ALL ITEMS AND CHECK ONLY RELEVANT CHECKBOXES BELOW
Auto review
Project management
priority:andpr:labels been added to the pull request? (In case of doubt, start with the labelspriority: lowandpr: to review later)area:,type:)Changelog and release notes
CHANGELOG.adoc+doc/content/modules/user-manual/pages/release-notes/YYYY.MM.0.adocbeen updated to reference the relevant issues?CHANGELOG.adoc?CHANGELOG.adoc?doc/content/modules/user-manual/pages/release-notes/YYYY.MM.0.adoc?Key highlightssection indoc/content/modules/user-manual/pages/release-notes/YYYY.MM.0.adoc?Documentation
Tests