Skip to content
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

Add support for MapTiler SDK #199

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Add support for MapTiler SDK #199

wants to merge 9 commits into from

Conversation

petr-hajek
Copy link

@petr-hajek petr-hajek commented Sep 16, 2024

Description

As a user of MapTiler SDK & MapTiler maps I'd like an easy way how to compare also MapTiler maps.
With this PR I can add MapTiler API key to the config (similar to MapBox). I can also use MapTiler SDK as a renderer (it's almost the same like MapLibre).

I removed the duplicate code in Map.svelte

QA steps

Probably some basic regression of MapLibre & MapBox but should not be affected

Author checklist

Create the PR

  • Fill out PR template
  • Make sure you've added a CHANGELOG entry under "Unreleased"
  • Request a review
  • Make any requested changes and get approval

After approval

  • Merge any changes from main into your branch, resolve any conflicts
  • Squash & merge changes into main
  • Delete branch

@petr-hajek petr-hajek marked this pull request as draft September 16, 2024 14:54
@petr-hajek petr-hajek marked this pull request as ready for review September 16, 2024 16:05
@ebrelsford
Copy link
Contributor

@petr-hajek Thanks for this! We'll be taking a look at this soon.

@petr-hajek
Copy link
Author

It's my first PR into this project and for full transparency, MapTiler is my client so I am open to any feedback. Thanks a lot. Anyway, this is a great tool!

Copy link
Collaborator

@aparlato aparlato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good here!

I left a long explainer for this, but basically I think we may need to switch the order of our || conditional here to be style || url as opposed to how it is now.

We'll also want to QA that change and make sure there's no weird side effects.

cc @ebrelsford

src/fetch-url.js Outdated Show resolved Hide resolved
@petr-hajek
Copy link
Author

Sorry for not responding. I was loaded with other things, but I'll try get back to this soon, thanks for the feedback.

@petr-hajek
Copy link
Author

Sorry it took me so long.

I moved the url normalization to a separate file and apply it in the GlMap component as well. It now works with Maplibre and Mapbox renderer as well.

@petr-hajek petr-hajek requested a review from aparlato December 20, 2024 13:09
@@ -50,7 +51,9 @@
let mapViewProps = {};

// We can set style (an object) here because mapStyle only changes when it needs to
$: ({ style, url } = mapStyle);
$: ({ style, url: baseUrl } = mapStyle);
$: url = normalizeMapTilerUrl(baseUrl, $configStore.maptilerApiKey);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this works, but feels like it isn't totally following existing patterns. By adding a Maptiler key property in our config, we're treating these URL's more similar to a Mapbox URL despite the fact it doesn't have a custom Maptiler protocol (in the way Mapbox has a custom protocol). In theory, if we were to treat Mapbox URLs like this, we would normalize all Mapbox URL's here as well and allow them to also be rendered by Maplibre or Maptiler.

I'm debating whether to:

  • move forward with this with a comment to rethink how this pattern works
  • apply the same logic to Mapbox URL's to allow them to run on different renderers
  • or force the user to either use the Maptiler SDK with this pattern or use the URL with key as a param when wanting to use other renderers.

@ebrelsford can you chime in here with your thoughts on the pattern? Let me know if my explanation here isn't clear enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants