maptiler / maptiler-sdk-js

Maps SDK tailored for MapTiler Cloud powered by MapLibre GL JS
https://docs.maptiler.com/sdk-js/
BSD 3-Clause "New" or "Revised" License
79 stars 14 forks source link

Changing MapTilerNavigationControl options have no effect #91

Closed ThatIsAPseudo closed 1 month ago

ThatIsAPseudo commented 4 months ago

maptiler-sdk-js version: https://github.com/maptiler/maptiler-sdk-js/releases/tag/v2.1.0

Bug description

When creating a MaptilerNavigationControl the options given are not taken into account, whereas the MapTiler documentation says the constructor takes an options object as argument.

let map = new maptilersdk.Map({
    container: 'mapContainer',
    // ...
    navigationControl: false,
});

const nav = new maptilersdk.MaptilerNavigationControl( { showCompass: false } );
map.addControl(nav, 'top-right');
// Bug : compass is still showing

Moreover :

console.log(nav.options)
/* 
// Returns :
Object { showCompass: true, showZoom: true, visualizePitch: true }
// Should be :
Object { showCompass: false, showZoom: true, visualizePitch: true }
*/

Problem

When creating a MaptilerNavigationControl you should be able to pass a NavigationControlOptions to define showCompass, showZoom, visualizePitch.

Solution

https://github.com/maptiler/maptiler-sdk-js/blob/3588c8e55c4e1b253e70b544191be998e93b7391/src/MaptilerNavigationControl.ts#L8-L13

This should instead be something like

  constructor(options: NavigationControlOptions = { showCompass: true, showZoom: true, visualizePitch: true, }) {
    super({
      showCompass: options.showCompass,
      showZoom: options.showZoom,
      visualizePitch: options.visualizePitch
    });
ThatIsAPseudo commented 4 months ago

After some testing it needs some more tweaking this part : https://github.com/maptiler/maptiler-sdk-js/blob/3588c8e55c4e1b253e70b544191be998e93b7391/src/MaptilerNavigationControl.ts#L15-L35 with this :

if (this._compass) {
// original code
}

Then it seems to work as expected

jonathanlurie commented 4 months ago

Hello @ThatIsAPseudo , Thanks for your feeback. Originaly, the idea of MaptilerNavigationControl was to provide something simple with no confusing options and that just does the minimum, and you're right, the documentation is not aligned with this and shows the options of the parent class MaptilerNavigation instead. The best would probably be that it accepts the same options though so I'll make a ticket for this.

jonathanlurie commented 1 month ago

Hello, sorry for the late feedback, I kinda forgot to post the update here but this has been fixed