feat: persist layout state in localStorage #891
Conversation
marstamm
commented
Mar 16, 2023
marstamm
commented
| @@ -314,7 +401,7 @@ describe('<BpmnPropertiesPanel>', function() { | |||
| createBpmnPropertiesPanel({ container, eventBus }); | |||
|
|
|||
| // then | |||
| expect(loadedSpy).to.have.been.calledOnce; | |||
| expect(loadedSpy).to.have.been.called; | |||
There was a problem hiding this comment.
This basically tested internal API of the properties-panel before. I moved the assumption of calledOnce to the properties-panel repo, cf. bpmn-io/properties-panel#228
bpmn-io-tasks
bot
added
needs review
marstamm
requested review from
a team,
philippfromme and
barmac
and removed request for
a team
src/hooks/useSavedLayout.js
Outdated
|
|
||
| // helpers ////// | ||
| function getSavedLayout() { | ||
| const savedLayout = window.localStorage.getItem('bpmn-properties-panel-layout') || '{}'; |
There was a problem hiding this comment.
I'm not sure if we want to hard code this inside the properties panel.
I'd rather
- Provide a clear api / contract to listen for properties panel layout changes
- Instantiate the properties panel or update it with a shared layout
➡️ Be API first, and not bury this inside the properties panel. How saving happens (if remotely or locally) is an application concern I think.
There was a problem hiding this comment.
propertiesPanel.on('layoutChanged', ({ layout }) => {
// persist layout somewhere
});
propertiesPanel.setLayout(layout);There was a problem hiding this comment.
If we provide such a hook then could we make saving and getting configurable to accomplish https://github.com/bpmn-io/bpmn-js-properties-panel/pull/891/files#r1140288946?
There was a problem hiding this comment.
I agree that the API should be explicit. I added a172bd0 to make the API more explicit and also expose it as propertiesPanel.setLayout().
I disagree that the implementation should always be on the integration side. In my opinion, it makes sense to have sensible defaults in the core which can be overwritten by the application. This is the case here: you can implement your own persistence/update logic by subscribing to propertiesPanel.layoutChanged and using the API (either eventBus or propertiesPanel.setLayout).
If you don't implement your own handler, you still get basic persistence across tabs in localStorage as the default implementation.
There was a problem hiding this comment.
Let's say that this would be the first time that we move persistent state inside our libraries :).
There was a problem hiding this comment.
Fair point. Removed with c82d5b3
marstamm
force-pushed
the
persist-group-state
branch
3 times, most recently
from
c82d5b3
to
e712f6e
Compare
|
My last commit on Friday broke the tests, it is fixed now :) Ready for review again |
src/render/BpmnPropertiesPanel.js
Outdated
| @@ -207,7 +221,7 @@ export default function BpmnPropertiesPanel(props) { | |||
| headerProvider={ PanelHeaderProvider } | |||
| placeholderProvider={ PanelPlaceholderProvider(translate) } | |||
| groups={ groups } | |||
| layoutConfig={ layoutConfig } | |||
| layoutConfig={ layout } | |||
There was a problem hiding this comment.
Would make sense to change this key to layout, too?
Otherwise let's stay consistent and keep layout named layoutConfig.
|
actually, let's move this to WIP until bpmn-io/properties-panel#228 is merged and released. We intermittently removed the feature this PR relies on with |
bpmn-io-tasks
bot
added
in progress
- exposes `setLayout` on `propertiesPanel` module related to camunda/camunda-modeler#2638
marstamm
force-pushed
the
persist-group-state
branch
from
ba97191
to
ed334c9
Compare
bpmn-io-tasks
bot
added
needs review
