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.

Sign up for GitHub

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

Jump to bottom

Add touch support for NavigationControl's compass button + add pitch indication to icon #8226

Merged
merged 7 commits into from
May 28, 2019
Merged

Add touch support for NavigationControl's compass button + add pitch indication to icon #8226

merged 7 commits into from
May 28, 2019

Conversation

pakastin
Copy link
Contributor

@pakastin pakastin commented May 8, 2019

Related to #3405

NavigationControl's compass button was missing touch drag support. Added that.

You can try the functionality here: https://aviamaps.com/map

@pakastin
Copy link
Contributor Author

pakastin commented May 8, 2019

Tapping broke, will fix that now

@pakastin
Copy link
Contributor Author

pakastin commented May 8, 2019

Tapping bug now fixed, so everything is working as expected!

@pakastin
Copy link
Contributor Author

pakastin commented May 8, 2019

Linting issues now fixed!

@pakastin
Copy link
Contributor Author

pakastin commented May 8, 2019

Bonus feature: compass icon now pitches when pitch is changed

@pakastin pakastin changed the title Add touch support for NavigationControl's compass button Add touch support for NavigationControl's compass button + add pitch indication to icon May 8, 2019
@pakastin
Copy link
Contributor Author

pakastin commented May 8, 2019

Both features running smoothly: https://aviamaps.com/map 😎

@andrewharvey
Copy link
Collaborator

Works nicely! Do you think clicking to reset should also reset the pitch?

@pakastin
Copy link
Contributor Author

pakastin commented May 8, 2019 •
edited
Loading

I was thinking about that but not sure.. Because for example when you're in a "navigation mode", it would make sense to only reset rotation 🤔

@andrewharvey
Copy link
Collaborator

I'm leaning towards it should reset pitch as well.

If this change is accepted, then I think we should update the API docs at https://docs.mapbox.com/mapbox-gl-js/api/#navigationcontrol to make it clear that the compass controls both bearing and pitch.

@cs09g
Copy link
Contributor

cs09g commented May 8, 2019

The second feature is also discussing on #8208. I think one PR should be covered one feature.

@pakastin
Copy link
Contributor Author

pakastin commented May 8, 2019

The second feature is also discussing on #8208. I think one PR should be covered one feature.

that was the plan, but accidentally came into this PR.. 😕

@cs09g
Copy link
Contributor

cs09g commented May 9, 2019

How could you skip the issue already registered. I'd like you to divide this PR for each feature.

@andrewharvey
Copy link
Collaborator

How could you skip the issue already registered. I'd like you to divide this PR for each feature.

@pakastin I think it's okay for both these features to be done in this single PR given they are related, and the touch support is fairly minor. I don't think it's worth the overhead of effort separating them, unless you want to, or unless the maintainers ask...

@pakastin
Copy link
Contributor Author

pakastin commented May 9, 2019

Ok, perfect! They're both basically patches IMO..

@pakastin
Copy link
Contributor Author

Any news on merging this?

ryanhamley
ryanhamley approved these changes May 28, 2019
View reviewed changes
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.

Looks good to me. Thanks!

@ryanhamley ryanhamley merged commit 8e9a34b into mapbox:master May 28, 2019
@ryanhamley ryanhamley mentioned this pull request May 28, 2019
Closed
@pakastin
Copy link
Contributor Author

Good, I can update to v1.0.0 then 😎

@pakastin pakastin mentioned this pull request May 29, 2019
Closed
@mourner mourner mentioned this pull request May 29, 2019
Merged
mourner added a commit that referenced this pull request May 29, 2019
@mourner
Revert "Add touch support for NavigationControl's compass button + ad…
959c9fe 
…d pitch indication to icon (#8226)"

This reverts commit 8e9a34b.
mourner added a commit that referenced this pull request May 29, 2019
@mourner
Revert broken compass touch support; fix flow issues and passive even…
a67c213 
…ts on iOS (#8292)

* Revert "Add touch support for NavigationControl's compass button + add pitch indication to icon (#8226)"

This reverts commit 8e9a34b.

* fix passive events regression on iOS
@pakastin pakastin mentioned this pull request May 29, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cs09g cs09g cs09g left review comments

@asheemmamoowala asheemmamoowala asheemmamoowala left review comments

@ryanhamley ryanhamley ryanhamley approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pakastin @andrewharvey @cs09g @ryanhamley @asheemmamoowala