mapbox / mapbox/mapbox-gl-js

Split functionality of TouchZoomRotateHandler

Open
#4,395 3 comments 5 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

api :memo: feature :green_apple: needs discussion :speech_balloon:
Dominant language
TypeScript
Stars
12.4k
Forks
2.4k
PR merge metrics
No merged PRs in 30d

Description

## Motivation
Currently the [TouchZoomRotateHandler](https://github.com/mapbox/mapbox-gl-js/blob/b9e10b939c6a3fe5d7ecac209f751b4871970ede/src/ui/handler/touch_zoom_rotate.js) handles two distinct, but related, types of interaction: touch rotate, and touch rotate + zoom. Touch rotate + zoom is configurable when creating a map by setting the property `touchZoomRotate`, but touch rotate is only accessible by invoking `map.touchZoomRotate.disableRotate()`. This results in a confusing an inconsistent API because touch rotate is distinct from touch zoom + rotate, yet is only configurable via an API that shares a name with the latter.

## Design Alternatives
There are two ways I could see fixing this API issue.

1. Update the initialization of `TouchZoomRotateHandler` to accept a parameter like `canRotate`. In [enable()](https://github.com/mapbox/mapbox-gl-js/blob/b9e10b939c6a3fe5d7ecac209f751b4871970ede/src/ui/handler/touch_zoom_rotate.js#L52) simply add a line to read `options.canRotate` and set `this._rotationDisabled`.

2. Split touch rotate and touch rotate + zoom into two different event handlers. Create a new handler named `TouchRotateHandler`, which could be an extension of `TouchZoomRotateHandler`.

## Design
While option 1 is the easiest to implement, option 2 would create a cleaner API in my mind, as it would make it easier to set a property like `touchRotate` while initializing a map. This language and behavior would be more consistent with the rest of Mapbox GL JS.

### Mock-Up
If the proposed change was made, disabling touch rotation on a map would occur by setting the property `touchRotate` to `false` during map creation, or by invoking `map.touchRotate.disable()`.

### Concepts
As previously mentioned, the primary motivation for this change is to create a more consistent terminology and API. The current behavior is a bit of an outlier and difficult to discover.

Is this a change the Mapbox GL JS team would be interested in making? If so, I could submit a pull request proposal.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reading src/ui/handler/touch_zoom_rotate.js, especially enable(), and the map initialization options described in the issue. Resolve whether to add a separate TouchRotateHandler or a canRotate option, then verify that touch rotation can be configured consistently at initialization and through a dedicated API.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
frontend
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.