[doc] Add ADR on improving diagram modals #4938
Conversation
5497b6e to
f25bccb
Compare
Signed-off-by: Michaël Charfadi <michael.charfadi@obeosoft.com>
f25bccb to
3ec6c8b
Compare
|
|
||
| By wrapping the palette in a `ClickAwayListener` we could close the palette everytime we click outside the modal. | ||
|
|
||
| We would lose the fact that the palette is currently open when clicking outside the xyFlow pane. |
There was a problem hiding this comment.
I'm not sure that this is some expected behavior and not a bug so I don't think we want to keep this behavior because it makes the diagram palette a very specific dialog compared to all the other ones in the application.
|
|
||
| We then render it inside `rf__wrapper` (palette) or inside the `representation_area` (manage_visibility). | ||
|
|
||
| There is a decision that should be made as to what should be the dom element parent of the modal. |
There was a problem hiding this comment.
Why should both feature (regular palette vs manage visibility dialog) have the same parent?
|
|
||
| _Note that we currently use a custom portal component to render the palette in rf_wrapper._ | ||
|
|
||
| It could be better to put the modal in `the representation_area` if we want to reuse it in another representation in the future but there is no evidence that the behaviour of the modal is changing because `rf__wrapper` is the parent. |
There was a problem hiding this comment.
Why would we want to reuse any of those in another representation? My main concern with the palette would be for example to have a proper palette for the multi-selection but the palette and manage visibility dialog or using the palette else is nowhere near my radar.
| All modals have a header but we don't reuse a header component. | ||
|
|
||
| We also don't reuse the modal container and the logic attached to it. |
| ---- | ||
| return ( | ||
| <ModalWrapper initialPosition={position} height={200} width={150} closeDialog={closeDialog} isDraggable={true}> | ||
| <Stack> |
There was a problem hiding this comment.
Very minor comment but I'm really not fan of relying on basic MUI components like stack and grid instead of raw CSS (flexbox / grid) for the layout.
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: lowandpr: to review later)area:,difficulty:,type:)CHANGELOG.adocbeen 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.pngArchitectural 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 potentiallynullfor 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