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

DRD overview resizing fix #4781

Merged
merged 2 commits into from
Jan 15, 2025
Merged

DRD overview resizing fix #4781

merged 2 commits into from
Jan 15, 2025

Conversation

jarekdanielak
Copy link
Contributor

Proposed Changes

This pull requests fixes two problems I found when resizing DRD preview:

  1. When resizing, diagram drop zone would trigger occasionally and flash.
  2. When resizing on macOS, the default (unloaded image) icon would show next to the cursor.

Before:

Screen.Recording.2025-01-08.at.14.39.34.mov

After:

Screen.Recording.2025-01-08.at.14.40.03.mov

Checklist

To ensure you provided everything we need to look at your PR:

  • Brief textual description of the changes present
  • Visual demo attached
  • Steps to try out present, i.e. using the @bpmn-io/sr tool
  • Related issue linked via Closes {LINK_TO_ISSUE} or Related to {LINK_TO_ISSUE}

@jarekdanielak
Copy link
Contributor Author

@philippfromme could you have a look on Windows?

@barmac

This comment was marked as outdated.

@barmac
Copy link
Collaborator

barmac commented Jan 8, 2025

It still flickers for a bit. But why does it work for the panels, but not for the overview? Do we have competing implementations?

Screen.Recording.2025-01-08.at.17.33.23.mov

@jarekdanielak
Copy link
Contributor Author

The panels are using ResizableContainer, but this overview has its own implementation for draggable. I will check it can be reworked with ResizableContainer without too much hacking.

@jarekdanielak jarekdanielak marked this pull request as draft January 8, 2025 19:24
@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Jan 8, 2025
@jarekdanielak
Copy link
Contributor Author

It seems our ResizableContainer is not really suited to hold another Canvas inside of it.

I had a brief look at how resizing is implemented in the panels and columns of DMN tables. They are both built without utilizing HTML Drag and Drop API. I think HTML dragging is not the best choice to build a resizable container, but rewriting it would have minimal benefit for UX.

I suggest we merge this small improvement and leave this topic for now. If you want to leave it untouched until somebody comes to give it a proper re-work, fine by me.

@jarekdanielak jarekdanielak marked this pull request as ready for review January 9, 2025 15:00
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Jan 9, 2025
Copy link
Collaborator

@barmac barmac left a comment

Choose a reason for hiding this comment

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

OK let's accept the state as is. The PR is already an improvement, even though it's not the perfect solution.

@philippfromme
Copy link
Contributor

philippfromme commented Jan 13, 2025

I think HTML dragging is not the best choice to build a resizable container, but rewriting it would have minimal benefit for UX.

I agee, we are using it mainly for historical reasons because this utility has been around for a long time.

For the resizable panels (e.g. properties panel) we're not using it anymore.

@philippfromme philippfromme merged commit 0c39640 into main Jan 15, 2025
12 of 14 checks passed
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Jan 15, 2025
@philippfromme philippfromme deleted the dmn-overview-fix branch January 15, 2025 08:30
@github-actions github-actions bot added this to the M85 milestone Jan 15, 2025
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.

3 participants