Make compass and zoom controls optional #5348
Conversation
There was a problem hiding this comment.
Thanks for this contribution @matijs !
Looks good overall, just a couple of small changes needed.
src/ui/control/navigation_control.js
Outdated
| @@ -15,6 +15,9 @@ const defaultOptions = { | |||
| * A `NavigationControl` control contains zoom buttons and a compass. | |||
| * | |||
| * @implements {IControl} | |||
| * @param {Object} [options] | |||
| * @param {Object} [options.showCompass=true] If `false` will omit the compass button from the Navigation Control. The default is `true` to show a compass button. | |||
There was a problem hiding this comment.
Small edit:
@param {boolean} [options.showCompass=true] If
true, the compass button is included.
@param {boolean} [options.showZoom=true] Iftrue, the zoom-in and zoom-out buttons are included.
Note the type should be marked {boolean}, not {Object}.
The default value is shown in the generated docs, so no need to state it.
src/ui/control/navigation_control.js
Outdated
| if (this.options.showCompass) { | ||
| this._compass = this._createButton('mapboxgl-ctrl-icon mapboxgl-ctrl-compass', 'Reset North', () => this._map.resetNorth()); | ||
| this._compassArrow = DOM.create('span', 'mapboxgl-ctrl-compass-arrow', this._compass); | ||
| } |
There was a problem hiding this comment.
If the this._compass element is not created for showCompass: false, then we should also make sure in onAdd() not to create the DragRotateHandler, whose constructor accepts that element as an argument.
There was a problem hiding this comment.
I thought the same, but if no element it passed in the DragRotateHandler will default to the canvasContainer :)
It would take a bit more refactoring to uncouple what's currently quite tightly coupled. Especially not to break stuff that's already out there.
There was a problem hiding this comment.
if no element it passed in the DragRotateHandler will default to the canvasContainer
Ah, I see! Good point... but even so, I think we should avoid it: that default behavior of DragRotateHandler isn't documented as part of its internal API, and it could easily end up being removed in a refactor.
Especially not to break stuff that's already out there.
What do you see as potentially breaking?
There was a problem hiding this comment.
As @anandthakker suggests, we do need this change. In addition to the potential future compatibility issues, there is also the fact that the map creates and registers a DragRotateHandler of its own. If we let the navigation control create a second one bound to the same element, that will result in duplicate event handlers and probably produce unexpected behavior.
|
@anandthakker is there anything else I need to do for this? :) |
|
Hi @matijs - yes, this comment #5348 (comment) still stands -- the |
|
@anandthakker Ugh… I'm so sorry. That conversation was hidden from me by GitHub 😔 I'll probably have some time to look into this later this week. |
|
No problem - thanks for following up! |
Instead of having every control button have a bottom border, make stacked buttons have a top-border. This gets rid of the :last-child selector too.
The compass button of the navigation control is now optional. Creating a
new navigation control with `{showCompass: false}` will leave out the
compass button. Defaults to showing the compass button.
They still default to showing, but make them optional.
- Remove check from `_rotateCompassArrow` - Add checks to `onAdd` and `onRemove`
|
@anandthakker finally got round to fixing this :) |
Add options to
NavigationControl.showZoomandshowCompassboth default totruecan be used to hide either the zoom buttons or the compass button from the Navigation Control.Fixes #1554