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

[maps] keydown+scroll to zoom #135330

Merged
merged 16 commits into from
Jun 30, 2022
Merged

[maps] keydown+scroll to zoom #135330

merged 16 commits into from
Jun 30, 2022

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Jun 28, 2022

Fixes #43640

PR adds control+scroll to zoom when map is embedded. Map displays instructions when scrolled

I used shift because scroll by bounding box also used shift so I wanted to be consistent with that action
Using control to be consistent with other mapping implementations.

Screen Shot 2022-06-28 at 8 30 43 AM

@nreese nreese marked this pull request as ready for review June 28, 2022 21:22
@nreese nreese requested review from a team as code owners June 28, 2022 21:22
@nreese nreese added [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.4.0 labels Jun 28, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

@nreese nreese added release_note:enhancement backport:skip This commit does not require backporting labels Jun 28, 2022
@nyurik
Copy link
Contributor

nyurik commented Jun 28, 2022

Per my comments during the meeting - in my personal experience, most sites have used CTRL key rather than SHIFT. Shift is often used for various bbox selections. This is mostly a consistency thing rather than a strong preference, and other's experience could be different from mine.

@monfera
Copy link
Contributor

monfera commented Jun 29, 2022

I usually see the Command modifier if it requires a modifier key and don't remember seeing the Shift modifier.

What's currently called dashboard in Kibana is not a dashboard, it's an interactive document. In the future, it could evolve toward

  • a real dashboard that is not scrolling (see the GAH mock by @ryankeairns), maybe unifying with presentations which don't scroll either (basically, single-page presentation with a tiling layout)
  • a notebook format, which can scroll vertically, and, besides visualizations, may contain editable formulas, value entry (like filters) and, if the visibility of editable formulas/config is off, it regresses into an interactive Report document

A real dashboard should have no need for modifier keys, just like going to Google Maps doesn't need it and it's good.

The need for the modifier key only arises due to the conflicting scrolling functionality, and it's better to remove the conflict than to rely on feature discoverability.

I think we should guide users to avoid scrolling "dashboards" even right now. There are numerous UX problems with scrolly dashboards even putting this aside, such as possibly integral charts or maps scrolling out of page, or missing important signals. Change my mind 😄

A modifier key has a price, more tedious use of something that's more integral to the data experience (zooming) than scrolling an entire dashboard. And there needs to be feature discoverability, which won't be perfect.

Users can handle the capture. It's sometimes the case that there is a scrollable list on a document, that doesn't need a modifier key either. Users know how to deal with it.

Foregoing a modifier key solves the discoverability issue as well. A modifier key needs to be advertised; some tools add a map overlay if they encounter a zoom attempt without the modifier key, which is I think a UX with heavy tradeoffs.

@jsanz
Copy link
Member

jsanz commented Jun 29, 2022

just like going to Google Maps doesn't need it and it's good.

Not always, for mobile screens they request users to use two fingers as described here

image

There is a Leaflet plugin to get the same behavior using Control/Command key or two fingers for mobile devices.

I'll share more examples on how this is handled to support one modifier or the other, if I found them.

@nickpeihl
Copy link
Member

Google Maps uses ctrl+scroll on Windows and Linux and cmd+scroll on Mac to zoom.

Mapbox also uses ctrl+scoll (cmd+scroll on Mac) for zooming.

Maplibre should be soon adding support for cooperative gestures. The pending PR will also use ctrl+scroll (cmd+scoll on Mac) to zoom.

OpenLayers supports scroll zoom with a modifier key. It appears ctrl (cmd on Mac) is the default modifier.

As @jsanz noted, there is a plugin for Leaflet that supports cooperative gestures. It also defaults to ctrl+scroll (cmd+scroll on Mac) for zoom.

ArcGIS is the oddity of this group. Scrolling on their webmaps pans the map North and South. 😖 However, shift+scroll will zoom the map. 🤷

I would prefer to follow the more popular convention of using ctrl+scroll on Windows/Linux and cmd+scroll on Mac to enable zoom on a dashboard.

@nreese
Copy link
Contributor Author

nreese commented Jun 29, 2022

I would prefer to follow the more popular convention of using ctrl+scroll on Windows/Linux and cmd+scroll on Mac to enable zoom on a dashboard.

Thanks for looking at other implementations. Agree, let's be consistent. I will update the PR to use ctrl+scroll on Windows/Linux and cmd+scroll on Mac.

The need for the modifier key only arises due to the conflicting scrolling functionality, and it's better to remove the conflict than to rely on feature discoverability.

Scroll-less dashboards is a laudable goal. Until that goal is achieved, keydown+scroll is a big step forward in usability.

@nreese nreese changed the title [maps] shift+scroll to zoom [maps] keydown+scroll to zoom Jun 29, 2022
@nreese
Copy link
Contributor Author

nreese commented Jun 29, 2022

@elasticmachine merge upstream

@nreese nreese requested a review from nickpeihl June 29, 2022 18:30
Copy link
Member

@nickpeihl nickpeihl left a comment

Choose a reason for hiding this comment

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

lgtm! just one small nit.

code review and tested map embeddables in dashboard and SIEM with chrome.

Copy link
Member

@jsanz jsanz left a comment

Choose a reason for hiding this comment

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

Apart from the comment about the size of the text already noted in Nick's review , this looks great to me 👏.

@nreese nreese enabled auto-merge (squash) June 30, 2022 15:13
@nreese nreese disabled auto-merge June 30, 2022 15:13
Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

@nreese: Nice addition! I've left some style and markup suggestions below for your review. Let me know if you have any questions. Thanks!

nreese and others added 4 commits June 30, 2022 11:04
…_scroll_zoom/_index.scss

Co-authored-by: Michael Marcialis <michael@marcial.is>
…_scroll_zoom/_index.scss

Co-authored-by: Michael Marcialis <michael@marcial.is>
…_scroll_zoom/keydown_scroll_zoom.tsx

Co-authored-by: Michael Marcialis <michael@marcial.is>
…_scroll_zoom/keydown_scroll_zoom.tsx

Co-authored-by: Michael Marcialis <michael@marcial.is>
@nreese
Copy link
Contributor Author

nreese commented Jun 30, 2022

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Jun 30, 2022

Nice addition! I've left some style and markup suggestions below for your review. Let me know if you have any questions. Thanks!

Thanks for all the great changes. I have merged them and everything looks very polished now.

@nreese nreese requested a review from MichaelMarcialis June 30, 2022 17:10
Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for making those changes.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
maps 861 862 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
maps 2.6MB 2.6MB +1.7KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@nreese nreese merged commit 4a82f98 into elastic:main Jun 30, 2022
yakhinvadim pushed a commit to yakhinvadim/kibana that referenced this pull request Jul 5, 2022
* [maps] shift+scroll to zoom

* content

* only enable keydown scroll zoom in embeddable

* scss lint fix

* one more scss lint fix

* replace shift with control

* lint

* use smaller css transition so message hides faster when keydown+scrolling

* h3 and small text

* Update x-pack/plugins/maps/public/connected_components/mb_map/keydown_scroll_zoom/_index.scss

Co-authored-by: Michael Marcialis <michael@marcial.is>

* Update x-pack/plugins/maps/public/connected_components/mb_map/keydown_scroll_zoom/_index.scss

Co-authored-by: Michael Marcialis <michael@marcial.is>

* Update x-pack/plugins/maps/public/connected_components/mb_map/keydown_scroll_zoom/keydown_scroll_zoom.tsx

Co-authored-by: Michael Marcialis <michael@marcial.is>

* Update x-pack/plugins/maps/public/connected_components/mb_map/keydown_scroll_zoom/keydown_scroll_zoom.tsx

Co-authored-by: Michael Marcialis <michael@marcial.is>

* clean-up

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Michael Marcialis <michael@marcial.is>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation Feature:Maps release_note:enhancement v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Maps] [Embeddables] Enable map zoom with keyboard modifier + scroll
10 participants