-
-
Notifications
You must be signed in to change notification settings - Fork 411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Convert from Mapbox GL to MapLibre #812
Conversation
* Also removes specialized support for Mapbox URLs.
Thanks so much @kevinschaul! I'll take a look tomorrow. Super glad someone was able to pick up where I left off. 😄 |
I think we can probably drop tests and build for Node 12, 14, and 16 in favor of 18? Any objections? |
Great, thanks @lseelenbinder! Looks like build/artifacts is failing to install the maputnik desktop version. I had trouble installing the desktop version in January but solved it with this PR: maputnik/desktop#17 I wonder if merging that would resolve this build issue. |
@kevinschaul I think we should merge that PR first, although I don't see it passing any CI... perhaps because I missed it initially. That PR seems reasonable, but I'm not a GO expert |
The go part looks good to me @kevinschaul, as well. (Also not an expert, but I've done a fair share of work with it.) |
PR looks good. I'd like to see more passing CI, but that may be better to do on |
Apologies, I shared an older PR to maputnik/desktop. This PR fixes CI too. I think this is the better candidate to merge: maputnik/desktop#18 |
🥳 Passing tests! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks awesome! Is this ready to merge?
I think so! Do you have thoughts on the version number? We have it as |
CI/codesandbox is failing, and I suspect it can be solved with one of the ways to accept the host, e.g. |
Merged! I agree that 2.x is good for this kind of change :) |
@@ -1,3 +1,5 @@ | |||
/* TODO */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be worth moving to a separate / new issue, describing the TODO in more detail.
Picking up #789 where @lseelenbinder left it. The tests are now passing for me locally, and things seem to be working in the limited testing I've done manually.
Fixes #781.