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

No default theme for leafletLayer #153

Open
wginsberg opened this issue May 4, 2024 · 5 comments
Open

No default theme for leafletLayer #153

wginsberg opened this issue May 4, 2024 · 5 comments

Comments

@wginsberg
Copy link

The theme option for the leafletLayer function is marked as optional, however when I omit nothing gets shown on the map.

I.e.

// This works
protomapsL.leafletLayer({url:'FILE.pmtiles OR ENDPOINT/{z}/{x}/{y}.mvt',theme:"light"})

// This doesn't work
protomapsL.leafletLayer({url:'FILE.pmtiles OR ENDPOINT/{z}/{x}/{y}.mvt'})

I am keen to try to submit a PR to fix this one.

@bdon
Copy link
Member

bdon commented May 6, 2024

Should we throw an error if no theme is passed?

It could be more explicit if we had positional arguments. In the future, the lang argument will also be required to specify en, de, etc.

Proposal welcome.

@wginsberg
Copy link
Author

Just opened a PR where it just sets the theme to "light" by default. I think it is a little nicer than throwing an error (and maybe how this worked before? I was using an older version of this package and encountered this problem when I upgraded)

@bdon
Copy link
Member

bdon commented May 8, 2024

Thanks for the PR. I recall the issue here now, which is that if we pass this:

leafletLayer({paintRules:myPaintRules})

If I'm visualizing my own tileset, as of right now on main this sets labelRules to [] which makes sense. If we set a default theme it will make the labelRules default to the light theme label rules, which is also confusing and implicit behavior.

So we probably need some breaking change in the API to encapsulate these use cases:

  1. initialize the map with a default theme
  2. initialize the map with one of the pre-baked 5 themes
  3. initialize the map with a custom set of label rules / paint rules

@wginsberg
Copy link
Author

Fair enough! will close my PR for now

@bdon
Copy link
Member

bdon commented May 9, 2024

What if we created a 2nd entry point called like leafletBasemapLayer("light", {"url":"...."}) to encapsulate this use case?

Also, would a Typedoc page like this: https://protomaps.github.io/PMTiles/typedoc/ be helpful?

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

No branches or pull requests

2 participants