[doc] Add pitch for minimap default visibility#6302
Conversation
69f9b3b to
b4285c1
Compare
2dd2c2c to
f0c80b1
Compare
f0c80b1 to
2169af3
Compare
10004fb to
4b41004
Compare
|
I've updated the shape to fit the new format with pitches |
4b41004 to
aa66237
Compare
sbegaudeau
left a comment
There was a problem hiding this comment.
What about the existing values in local storage? How are they removed?
| :status: proposed | ||
| :consulted: Florian Rouëné | ||
| :informed: Florian Rouëné | ||
| :deciders: Stéphane Bégaudeau |
There was a problem hiding this comment.
- It's funny that I'm the decider yet I am not affected this PR nor asked to review it
- Commits for pitches and adrs uses the doc tag and are not assigned to the issue to not close it
- The pitch is not merged nor accepted yet the work seems to be done already
|
|
||
| == Problem | ||
|
|
||
| Currently, the minimap visibility is controlled by a global `localStorage` setting. If a user hides the minimap on one diagram, it remains hidden on all diagrams. Furthermore, diagram specifiers have no way to define a default state for their diagram via the View DSL. |
There was a problem hiding this comment.
One sentence per line (the same comment can be repeated for the whole file)
There was a problem hiding this comment.
No it's not done, far from it. I'll do it myself to make this pitch move forward.
|
|
||
| 1. **View DSL integration:** Add a property to the View DSL, on the `DiagramDescription` to set the initial `showMinimap` state. | ||
| 2. **State Lookup:** On diagram component mount, the app checks for a user preference in `localStorage` keyed by the diagram ID. If null, it falls back to the default View DSL value. | ||
| 3. **Removal of Global Key:** Deprecate the existing global `sirius-diagram-mini-map-visibility` key to prevent state pollution. |
There was a problem hiding this comment.
It is still written deprecate...
|
|
||
| boolean canHandle(IEditingContext editingContext, DiagramContext diagramContext, DiagramDescription diagramDescription); | ||
|
|
||
| boolean showMinimap(IEditingContext editingContext, DiagramContext diagramContext, DiagramDescription diagramDescription); |
There was a problem hiding this comment.
Why showMinimap? It does not show the minimap, it computes the minimap visibility.
There was a problem hiding this comment.
I propose minimapVisible
There was a problem hiding this comment.
It's a function, it should start with a verb, something like isMinimapVisible
There was a problem hiding this comment.
The service will have be named isMinimapVisible (I moved the code in another Draft PR) but the field in the graphQL API and description will be named minimapVisible. It is the case for autoLayout
aa66237 to
57f83ed
Compare
57f83ed to
222aac0
Compare
|
|
||
| == Rabbit holes | ||
| * **Diagram Identification:** If IDs are non-persistent (transient diagrams), the user preference will be lost. | ||
| * **Storage Bloat:** If a user views thousands of diagrams, `localStorage` will accumulate thousands of small keys. We should consider to delete the oldest keys when deleting a diagram. |
There was a problem hiding this comment.
This will require an answer at the technical level (I don't need an ADR for that but it will have to be fixed in the code), we can't just create stuff on the machine of the end users without any cleanup solution.
There was a problem hiding this comment.
The problem here (which also applies to the delete preference ask) is that someone else might have already deleted the project or representation.
There was a problem hiding this comment.
So, at the technical level, we will need at least to track the last time we had to interact with the data in question. If we did not use some local storage data for a year, we can probably delete it
222aac0 to
6c7c593
Compare
| * **Nice-to-have:** A "Reset to Default" button in the UI to clear the `localStorage` override. | ||
|
|
||
| == Rabbit holes | ||
| * **Diagram Identification:** If IDs are non-persistent (transient diagrams), the user preference will be lost. |
There was a problem hiding this comment.
Those are not related, the representation can be transient and its id stable over time
6c7c593 to
f7d1c1b
Compare
Bug: #6295 Signed-off-by: Florian Barbin <florian.barbin@obeosoft.com>
f7d1c1b to
f8bb85d
Compare
Bug: #6295
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