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] Allow to zoom the BPMN diagram with mouse wheel #734

Merged
merged 11 commits into from
Oct 15, 2020

Conversation

tbouffard
Copy link
Member

@tbouffard tbouffard commented Oct 12, 2020

Add zoom support with CTRL + mouse wheel for Linux and Windows.
As discussed with @csouchet, macOS support will be managed with #773.

closes #730

@tbouffard tbouffard added the depends on another PR ⚠️ Pull request depending on another one. The depending must be merged first label Oct 12, 2020
@tbouffard tbouffard changed the title zoom with mouse wheel [FEAT] Allow to zoom the BPMN diagram with mouse wheel Oct 12, 2020
@tbouffard tbouffard force-pushed the 730-zoom_with_mouse_wheel branch from 650e3b2 to aa65a4d Compare October 13, 2020 08:25
@tbouffard tbouffard added BPMN diagram usability Something about the way we can interact with BPMN diagrams rebase needed 💥 Pull request that must be rebased on the latest master commit prior being reviewed or merged and removed depends on another PR ⚠️ Pull request depending on another one. The depending must be merged first labels Oct 13, 2020
@tbouffard tbouffard force-pushed the 730-zoom_with_mouse_wheel branch from 2ee8cb4 to c41cb8f Compare October 14, 2020 09:42
@aibcmars aibcmars removed the rebase needed 💥 Pull request that must be rebased on the latest master commit prior being reviewed or merged label Oct 14, 2020
@csouchet
Copy link
Member

csouchet commented Oct 15, 2020

Mac OS

Firefox

  • CTRL + mouse wheel => OK, but Browser Zoom
  • CMD + mouse wheel => OK, but Browser Zoom

=> Browser Zoom, even the button is zoom/dezoom (cursor position on task)

Safari

  • CTRL + mouse wheel => KO
  • CMD + mouse wheel => KO

# Conflicts:
#	test/e2e/bpmn.navigation.test.ts
@tbouffard
Copy link
Member Author

@csouchet this macOS behaviour is strange. Automatic zoom tests are passing with Chromium. We will check this out in #773

@tbouffard tbouffard marked this pull request as ready for review October 15, 2020 09:02
@tbouffard tbouffard added the enhancement New feature or request label Oct 15, 2020
@@ -31,7 +31,8 @@ export default class BpmnVisualization {
}
// Instantiate and configure Graph
const configurator = new MxGraphConfigurator(this.container);
this.graph = configurator.configure(options);
this.graph = configurator.configure();
configurator.configureMouseNavigationSupport(options);
Copy link
Member

Choose a reason for hiding this comment

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

Why don't put this in configurator.configure() ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess I missed to reintegrate it after refactoring, the configureMouseNavigationSupport method was previously hold in BpmnVisualization
I will put everything within the MxGraphConfigurator.configure

Copy link
Member Author

Choose a reason for hiding this comment

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

Done with 8346db6

const deltaX = zoom === 'zoom in' ? -100 : 100;
// simulate mouse+ctrl zoom
await page.mouse.move(viewportCenterX, viewportCenterY);
await page.keyboard.down('Control');
Copy link
Contributor

@aibcmars aibcmars Oct 15, 2020

Choose a reason for hiding this comment

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

I forgot to check it but it seems it is working well on Mac in our GitHub 'checks' :)
perhaps puppeteer handles that

Copy link
Member Author

Choose a reason for hiding this comment

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

puppeteer is running Chromium. Let's review this in #773

Copy link
Contributor

@aibcmars aibcmars left a comment

Choose a reason for hiding this comment

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

LGTM

@tbouffard tbouffard merged commit c48462b into master Oct 15, 2020
@tbouffard tbouffard deleted the 730-zoom_with_mouse_wheel branch October 15, 2020 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BPMN diagram usability Something about the way we can interact with BPMN diagrams enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT] Zoom in and Zoom out with Ctrl+mouse wheel
3 participants