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
[2418] Extract synchronize with selection into a hook #2419
base: master
Are you sure you want to change the base?
Conversation
3be6959
to
b257a82
Compare
b257a82
to
7ea4569
Compare
CHANGELOG.adoc
Outdated
@@ -156,6 +158,7 @@ These color palettes are accessible in studio projects but are not visible in th | |||
- [ADR-104] Add support for Help Expressions in Form widgets | |||
- [ADR-105] Add custom widget to edit EMF references | |||
- [ADR-106] Add the support for the palette with react flow | |||
- [ADR-107] - Improve frontend selection support |
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.
- Should be moved to 2023.12 (and remove the " -" which is not present in the others.
- We're now at ADR 122, this would be (at least) ADR 123.
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.
Done
|
||
``` | ||
|
||
This makes sense because the selection is relative to a particular project/editing context. |
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.
Actually, maybe not always.
I am working on Portals support, where one I need more control on how the selection inside the representations displayed in a portal are synchronized with the global, project-wide selection.
This is just a remark, it seems that even with this new approach I should be able to locally override the selection handling for my case. It might be worth mentioning, you decide.
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.
Actually I see that the pattern is used in e.g. modals to get a local selection.
@@ -0,0 +1,75 @@ | |||
= ADR-107 - Improve frontend selection support |
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.
This does not mention the SynchonizeWithSelection API also added here.
|
||
const setSelection = useCallback((selection: Selection) => { | ||
const selectedRepresentation = selection.entries.filter(isRepresentation); | ||
setState((prevState) => ({ ...prevState, selection, selectedRepresentation })); |
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.
selectedRepresentation
is not declared in SelectionContextProviderState
.
If the frontend still compiles I guess it means it's not actually used anywhere?
I suppose it should be used in the workbench, but the code there continues to read the full selection and do its own filtering (equivalent to this one):
const representations: Representation[] = selection.entries.filter((entry) =>
entry.kind.startsWith('siriusComponents://representation')
);
@@ -170,9 +148,9 @@ export const workbenchMachine = Machine<WorkbenchContext, WorkbenchStateSchema, | |||
|
|||
const newSelectedRepresentations = [...representations, ...newRepresentations]; | |||
|
|||
return { selection, displayedRepresentation, representations: newSelectedRepresentations }; | |||
return { displayedRepresentation, representations: newSelectedRepresentations }; |
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.
It seems we're losing the test we had before which prevented "changing" the state with the same selected representations.
packages/trees/frontend/sirius-components-trees/src/views/ExplorerView.tsx
Outdated
Show resolved
Hide resolved
packages/sirius-web/frontend/sirius-web/src/views/edit-project/EditProjectView.tsx
Outdated
Show resolved
Hide resolved
toggleSynchronizeWithSelection(); | ||
}; | ||
|
||
const preferenceButtonSynchroniseTitle = isSynchronized |
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 code uses the US spelling ("synchroni[z]ed" with a "z") except for here (variable names an titles, use the UK spelling, with an "s").
35cbc34
to
e262c60
Compare
a30af8c
to
2ee28ac
Compare
I have some doubt about the implementation here, @sbegaudeau we should discuss about that |
5aeaf09
to
05b3848
Compare
05b3848
to
1faf644
Compare
Bug: #2418 Signed-off-by: Guillaume Coutable <guillaume.coutable@obeo.fr>
1faf644
to
42cfbaa
Compare
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