Skip to content
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

feat(bpmn): persist layout across sessions #3556

Merged
merged 2 commits into from
Apr 19, 2023
Merged

Conversation

marstamm
Copy link
Member

closes #2638

Recording 2023-04-14 at 10 10 56

@marstamm marstamm requested a review from a team April 14, 2023 08:15
@marstamm marstamm self-assigned this Apr 14, 2023
@marstamm marstamm requested review from philippfromme and barmac and removed request for a team April 14, 2023 08:15
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Apr 14, 2023
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering: Shall we also persist DMN and FORM properties panel state? I don't see a reason we should not.

Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that it works as expected. Good stuff!

However I found an annoying flicker that degrades the perceived utility of this feature: The app feels shaky, as demonstrated by the following screen capture (switching between two tabs):

capture NkyOJr_optimized

Is there a way we can avoid this? One way could be to update the properties panel layout pre-mount? Potentially other options exist.

@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Apr 18, 2023
@marstamm
Copy link
Member Author

It looks like it marks headers as sticky for one frame. My guess is that because we change the layout before mounting, the scroll position in an unmounted state is always "out of screen". I'll check if we can disable sticky headers checks when the properties panel is unmounted and if this fixes it

@barmac
Copy link
Contributor

barmac commented Apr 19, 2023

My gut feeling is some problems may come from this: https://github.com/bpmn-io/properties-panel/blob/main/src/hooks/useStickyIntersectionObserver.js

@nikku
Copy link
Member

nikku commented Apr 19, 2023

useLayoutEffect could be worth a try (cf. explaination).

@marstamm
Copy link
Member Author

I can confirm that this is not caused by applying the layout, In Camunda Modeler 5.10.0 opened Groups are also initially rendered as sticky:

Recording 2023-04-19 at 10 18 36

@marstamm
Copy link
Member Author

So you are both right @barmac and @nikku 🎉

  • The header is sticky, which is caused by the stickyIntersection hook
  • The Group is also rendered in it previous state for 1 render cycle, which is fixed by useLayoutEffect

Thank you for your inputs, the fixes in the properties panel are here: bpmn-io/properties-panel#238

marstamm added a commit to bpmn-io/properties-panel that referenced this pull request Apr 19, 2023
- bpmn-js-properties-panel@1.21.0
- @bpmn-io/properties-panel@1.8.1
@marstamm
Copy link
Member Author

I updated the branch with the changes made in the properties panel. Feel free to try it out again

Recording 2023-04-19 at 11 51 25

@marstamm marstamm requested a review from nikku April 19, 2023 11:20
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kinda still impressed how natural (aka intuitive, not bad) this feels. Thanks for getting that done @marstamm!

I also like how this naturally just works with connector templates:

capture xlYbcq_optimized


As a follow up in the UX department I'd suggest the following:

  • Introduce error indicators for closed sections
  • Hook up connector template errors with our linting infrastructure

@nikku
Copy link
Member

nikku commented Apr 19, 2023

Added @YanaSegal and @christian-konrad as reviewers so that both are in the loop here (non-blocking).

@nikku nikku merged commit 66a1241 into develop Apr 19, 2023
@nikku nikku deleted the save-opened-groups branch April 19, 2023 11:40
@bpmn-io-tasks bpmn-io-tasks bot removed the in progress Currently worked on label Apr 19, 2023
@christian-konrad
Copy link
Contributor

Great contribution, improving the QoL of both new and power users!

Hook up connector template errors with our linting infrastructure

We have an (soon to be defined) epic for it: https://github.com/camunda/product-hub/issues/498

Introduce error indicators for closed sections

I like that idea, remembered we discussed that quite a while ago @YanaSegal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Persist properties panel state across sessions
4 participants