Closed (fixed)
Project:
Leaflet More Maps
Version:
2.1.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
15 Dec 2023 at 13:55 UTC
Updated:
12 Jan 2024 at 16:24 UTC
Jump to comment: Most recent
Show commands
changes
Comments
Comment #2
petr illekHi,
adding initial MR with the required functionality.
Comment #3
ressaI see two branches, but no MR ... It looks like the
2.1.xbranch is the one to open an MR for?Comment #5
petr illek@ressa
Sorry for the confusion. MR now ready.
Comment #7
ressaThanks! ... and I just used the new #3189125: Allow hiding issue fork branches feature to hide the extra branch, very nice :)
Comment #8
ressaIt works great, thanks @Petr Illek. I did have to create an account to test it, and as part of the process send them an SMS ... Do you know if they offer a (very) limited development API key? It's not so important, but would be nice.
The "create an account" link looks funny, maybe it was copied from Mapbox, which also looks like it doesn't work? Perhaps you can simply remove
target="_access_tokenboth places?Comment #9
petr illekThanks for the reviewing almost in realtime! Highly appreciated.
You were right, I copied that piece from the Mapbox snippet. I looked at the others and it was everywhere. So I removed it as it does not make any sense. Not sure if there might by some JS (or other code) somewhere doing something according to the target value??
I've also done some updates on the Mapy.cz attribution so it should look ok in more situation (now it was in second line thanks to the Olivero styling).
To answer your question about the dev account. I don't know, but I'll be writing them soon, so I will ask. ;)
Comment #10
petr illekComment #11
ressaYou're welcome, and thanks for a fast update and fixing all the links. I agree, it's best to keep it simple. The maps still look great, though I did have to zoom in a lot on a city, to see the difference between Basic and Outdoor, but they are not identical. I didn't notice the Mapy.cz attribution before, but it looks great now.
Feel free to review these map additions and README update I added recently :)
Comment #12
petr illekI realize I forgot to update the Readme file.
Comment #13
rachel_norfolkI admit I might be picky but there appear to be a couple of changes in the MR that are outside the scope of the issue. A couple of changes of double quotes to single ones.
Now, I happen to agree with the change but it should be in an issue that details that change.
Could you update the MR to only include in-scope changes and set straight back to RTBC? I’ll then happily merge.
Ta
Comment #14
ressa@Petr Illek: FYI, I created #3410669: [Meta] Release version 2.2, and added this issue to the list as well.
Comment #15
petr illekThank you @rachel_norfolk for being picky,
I've reverted the changes and left there only the one regarding the newly added code.
A side note – and feel free to point me to a discussion if that was (and I'm sure it was) already being discussed before.
I must admit I on one hand like the strict approach with doing the code only for what is described in the issue. I understand the need for that as it is easier to do the CR. On the other hand if it is a merely a coding standard fix, does not this approach slow down the overall development?
Comment #17
rachel_norfolkTo be honest, Petr, the pickiness is more about me trying to improve my own practices than anything else. I’ve been quite guilty of proposing (and accepting!) MRs that include more than described.
Awesome new feature, though - more maps!!! 👏
Comment #18
petr illekThank you Rachel!
🥳