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

Track popup to cursor [ready] #7786

Merged
merged 8 commits into from
May 8, 2019
Merged

Track popup to cursor [ready] #7786

merged 8 commits into from
May 8, 2019

Conversation

peterqliu
Copy link
Contributor

@peterqliu peterqliu commented Jan 18, 2019

ticket: #7777

Adds a method .trackPointer() to the Popup API that repositions the popup to the mouse cursor on every mousemove event, and hides/shows the popup when cursor leaves/enters the map. Mutually overwritable with .setLngLat.

Design

Opted not to extend setLngLat, as it doesn't quite fit those words. Also opted not to extend the popup options object, for cases when users need to change behavior after adding to map.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • manually test the debug page
  • tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes

@ryanhamley
Copy link
Contributor

Touchscreen behavior: show popup only on touchmove? Not show at all? Console warning when invoking .trackCursor() on touch devices?

I'm trying to imagine a use case for this on mobile. It seems like it would be a very awkward UX as compared to simply touching on each new point to see a new popup. On the other hand, having a desktop only API method seems odd. Do we have other methods that don't work on mobile? If we do enable this on touchmove, maybe it would be better to call the method a more generic term like trackPointer.

Close button: won't be clickable when popup shadows mouse. Should we hide? Does that affect any corner cases?

This is a fun consequence. Maybe there needs to be a separate action to close the popup in this case. Would clicking the map to toggle the popup be too weird?

@peterqliu
Copy link
Contributor Author

peterqliu commented Jan 25, 2019

Mobile handling

In lieu of hover, I thought about adding cursor-tracked popups to a tap on the map. The interaction pattern for mobile is unfortunately quite complex: click is the event that typically corresponds to this, but is nondeterministic. On Webkit browsers where the user has also implemented a hover listener, click is invoked only on the second tap, and not a double-tap at that. Because of this unpredictability, I've removed touchscreen support entirely for pointer-tracked popups, until we can implement a more deterministic behavior given underlying browser constraints.

Other changes

  • renamed trackCursor to trackPointer, to reflect the mouse-specific compatibility
  • move the show/hide logic to CSS, greatly simplifying the event handler logic
  • opted to keep the closeButton by default, as is the current Popup default. The change is easy enough on the user's side

@KravMaguy
Copy link

Mobile handling

In lieu of hover, I thought about adding cursor-tracked popups to a tap on the map. The interaction pattern for mobile is unfortunately quite complex: click is the event that typically corresponds to this, but is nondeterministic. On Webkit browsers where the user has also implemented a hover listener, click is invoked only on the second tap, and not a double-tap at that. Because of this unpredictability, I've removed touchscreen support entirely for the time being.

Peter If you remove the touchscreen support how can we show the user information about the
individual rooms in the building on mobile? will I loose the tooltip functionality in this example on mobile:
https://codepen.io/kravmaguy/pen/zerMJx

@peterqliu
Copy link
Contributor Author

@KravMaguy that is correct, you will lose that functionality if you use a cursor-tracked popup on mobile. In these contexts, most apps display the information in a panel separate from the map: it's a much better practice given the limited screen space, and preserves visibility/interactivity of the map itself.

some examples ⬇️

img_3115
img_3117
img_3116

@peterqliu
Copy link
Contributor Author

@KravMaguy you can still unproject/setLngLat the popup on each touch event, to move the popup there

Copy link
Contributor

@ryanhamley ryanhamley left a comment

Choose a reason for hiding this comment

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

I think overall it looks fine but I think the behavior is a little odd.

  • If you move the mouse quickly enough, you can end up inside the popup. I'm not entirely sure what the answer here could be, if anything.

  • The cursor partially obscures the "spout" of the popup when it's top-aligned. Perhaps we should add a bit of padding to the cursor to move it down a bit and/or make the cursor into a pointer instead of the grabber hand.

  • If you pan the map, the popup stays in one spot. Maybe on pan, the popup should close? I could see that being unexpected though.

src/ui/popup.js Outdated Show resolved Hide resolved
@peterqliu
Copy link
Contributor Author

peterqliu commented Jan 30, 2019

If you pan the map, the popup stays in one spot. Maybe on pan, the popup should close? I could see that being unexpected though.

👍the popup should definitely follow the cursor

If you move the mouse quickly enough, you can end up inside the popup. I'm not entirely sure what the answer here could be, if anything.

Unfortunately this looks like a limitation of the browser, in that it emits mousemove events too slowly to keep up with the mouse's actual movement. Not sure if we can do anything here

The cursor partially obscures the "spout" of the popup when it's top-aligned. Perhaps we should add a bit of padding to the cursor to move it down a bit and/or make the cursor into a pointer instead of the grabber hand.

Agree that this is weird, but the specific solution seems like it would vary by user/use case/preference. I'm inclined to let people modify CSS as appropriate.

@ryanhamley
Copy link
Contributor

Agree that this is weird, but the specific solution seems like it would vary by user/use case/preference. I'm inclined to let people modify CSS as appropriate.

👍 I do think it makes sense to change the cursor from a grabber to a pointer when in trackPointer mode. The idea of this feature is to point at various locations on the map and get information about them, so the pointer feels semantically valid as well as it visually covering less of the popup.

@peterqliu
Copy link
Contributor Author

  • opted to hide popup when dragging the map -- tracking popup on drag made it harder to see the map when repositioning it takes priority

  • cursor is now pointer when tracking popup

@peterqliu
Copy link
Contributor Author

If you move the mouse quickly enough, you can end up inside the popup. I'm not entirely sure what the answer here could be, if anything.

Also removed interactivity with pointer-tracked popups, to avoid getting trapped in it 👍

@peterqliu peterqliu changed the title Track popup to cursor [not ready] Track popup to cursor Feb 6, 2019
@Plantain
Copy link

Is this likely to make it into a release in the near future? We'd like to use this feature rather than the workarounds in #7777

@peterqliu
Copy link
Contributor Author

@Plantain this branch is ready to the extent that I can tell -- looping in some reviewers to see if we can get this across the line.

Copy link
Contributor

@ryanhamley ryanhamley left a comment

Choose a reason for hiding this comment

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

Couple minor things but this looks good to me

src/ui/popup.js Outdated Show resolved Hide resolved
src/ui/popup.js Outdated Show resolved Hide resolved
@ryanhamley
Copy link
Contributor

This is too late to be in the next release of GL JS which comes out this week, but it should be in May's release

@peterqliu peterqliu force-pushed the track-popup branch 2 times, most recently from 91a250f to 2cd4525 Compare April 2, 2019 22:08
@peterqliu
Copy link
Contributor Author

clarified a bit of the code per #7786 (comment) and squashed as much as I can (not sure if I can squash past a previous pull from master?)

@ryanhamley
Copy link
Contributor

ryanhamley commented Apr 2, 2019

You don't have to squash ahead of time @peterqliu. Once all the checks have passed, just pressing the Squash and merge button will take care of everything. You'll see a text field with the previous commits listed as a description. I usually just delete everything in that field and make sure the commit message is descriptive.

@peterqliu peterqliu changed the title Track popup to cursor Track popup to cursor [ready] Apr 2, 2019
@peterqliu peterqliu merged commit dd2f519 into master May 8, 2019
@peterqliu peterqliu deleted the track-popup branch May 9, 2019 00:22
@mourner mourner mentioned this pull request Jul 23, 2019
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.

6 participants