Scale update units #6138
Scale update units #6138
Conversation
There was a problem hiding this comment.
Thanks for the contribution @ryanhamley !
This looks great to me, with just a couple of small changes needed in the unit test.
test/unit/ui/control/scale.test.js
Outdated
|
|
||
| function createMap() { | ||
| const container = window.document.createElement('div'); | ||
| config.ACCESS_TOKEN = 'pk.123'; |
There was a problem hiding this comment.
The dummy access token shouldn't be necessary, and nor should the owner or id properties below. (I think it exists in the AttributionControl tests because that specific control needs it.)
|
|
||
| t.equal(map.getContainer().querySelectorAll('.mapboxgl-ctrl-top-left .mapboxgl-ctrl-scale').length, 1); | ||
| t.end(); | ||
| }); |
There was a problem hiding this comment.
🙇 thank you for adding these tests! Could you also add one that tests the new updateUnit() method?
There was a problem hiding this comment.
Hi @anandthakker I will work on this in the next day or so. Thanks for the feedback.
|
👍 to implementing this feature. an open question, should the naming of this method be |
👍 yep, agreed @andrewharvey -- I meant to mention that in the review, in fact! |
|
I'll update the name of the method to |
|
@anandthakker @andrewharvey I pushed the requested changes. |
src/ui/control/scale_control.js
Outdated
| /** | ||
| * Set the scale's unit of the distance | ||
| * | ||
| * @param {String} unit |
There was a problem hiding this comment.
String is causing the CI to fail, I believe it should be string.
There was a problem hiding this comment.
Also I think it would be helpful to include the documentation for unit, listing out what the supported values are.
|
I fixed the param documentation and it looks like the build ci is passing now @andrewharvey |
|
@anandthakker i added the requested test and everything is building now |
There was a problem hiding this comment.
Thanks @ryanhamley ! One more (hopefully small) testing change 🙏
test/unit/ui/control/scale.test.js
Outdated
|
|
||
| t.equal(scale.options.unit, 'metric'); | ||
| scale.setUnit('imperial'); | ||
| t.equal(scale.options.unit, 'imperial'); |
There was a problem hiding this comment.
The fact that the unit is stored in scale.options.unit is an implementation detail that could be changed in a future refactor. Let's update this test so that it wouldn't be affected by such a change by having these assertions actually check the content of the rendered scale control, e.g. maybe by searching for an appropriate string within map.getContainer().querySelectorAll('.mapboxgl-ctrl-top-left .mapboxgl-ctrl-scale').innerHTML
|
I pushed a change to the scale test that reads the unit from the scale's HTML @anandthakker Does that look good? I'm pretty sure it's not best practice to abstract getting the unit from the map into a function but I wasn't sure. |
test/unit/ui/control/scale.test.js
Outdated
| t.equal(scale.options.unit, 'metric'); | ||
| let unit = map.getContainer().querySelectorAll('.mapboxgl-ctrl-bottom-left .mapboxgl-ctrl-scale')[0].innerHTML.match(/[a-zA-Z]+|[0-9]+/g)[1]; | ||
|
|
||
| t.equal(unit, 'km'); |
There was a problem hiding this comment.
@ryanhamley small nitpick: let's simplify this just a tad to:
let contents = map.getContainer().querySelector('.mapboxgl-ctrl-bottom-left .mapboxgl-ctrl-scale').innerText; // (not 100% sure jsdom supports innerText, but worth a shot
t.match(contents, /\bkm\b/);
|
@anandthakker I pushed a simpler version of the test. it doesn't appear that jsdom has innerText so I used innerHTML and the word boundary character in the regex doesn't distinguish between letters and numbers so I had to remove that from the regex to match the correct characters. |
|
--> #6274 |
Potential fix for #6137
Launch Checklist
Adds a method to the ScaleControl to update the scale's unit of distance on the fly