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

Use map tiles dark mode without leaflet-osm plugin #5397

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

hlfan
Copy link
Contributor

@hlfan hlfan commented Dec 13, 2024

Like #5396 but without involving leaflet-osm.
Again, since embeds carry no color scheme preferences, I skipped the partially redundant implementation there.

Copy link
Collaborator

@gravitystorm gravitystorm left a comment

Choose a reason for hiding this comment

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

I'm very happy to see this work, thanks @hlfan!

The code looks fine to me, but I'm not a javascript expert so maybe there are better ways to write parts.

Two other changes requested:

  • Please rework your commits (e.g. using git rebase -i) to avoid having the fixup commits (e.g. combine the linting fixes into the original commits)
  • There's still a darken filter applied to the map previews - you can see that the colour of the green spaces on the transport dark map are darkened compared to the main map

Screenshot from 2024-12-18 11-31-42

const container = layer.getContainer();
if (!container) return;
if (layer.options.schemeClass) container.classList.add(layer.options.schemeClass);
const filterRecievers = [container];
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor spelling error - filterReceivers

Copy link
Member

@tomhughes tomhughes left a comment

Choose a reason for hiding this comment

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

This has gone rather further than I intended from my comments on #5396 by moving everything back into the web site code.

My thinking was the leaflet-osm would still have the darkUrl attribute on the layer, and probably an option to the constructor that would cause it to be used if available, and only if that wasn't possible would the web site code activate a filter instead.

The only thing I'm not sure about is how to signal back to this code whether dark more was possible.

@hlfan
Copy link
Contributor Author

hlfan commented Dec 18, 2024

@tomhughes This is also further than I anticipated it would go.

But I found it too hacky to overwrite the class's properties from the instance each time, and I especially wanted to avoid using __proto__. Yet I couldn't find a way to do that.

But then I stumbled upon #5386 and its new layer definition handler that allowed me to eliminate those hacks since this would specify the scheme on the instance creation.

Also not involving leaflet-osm at all would increase the flexibility with things like SVG filters, where I don't see an easy option to have that sourced from JS nicely.

@tomhughes
Copy link
Member

tomhughes commented Dec 18, 2024

But I found it too hacky to overwrite the class's properties from the instance each time, and I especially wanted to avoid using __proto__. Yet I couldn't find a way to do that.

I don't see any reason why what I was suggesting would need any of that but I've actually come up with a better solution now.

What really bothered me was having the URL sometimes in leaflet-osm and sometimes here, so what I've done is to add a new TransportDarkMap layer to leaflet-osm in openstreetmap/leaflet-osm@ebfaf22.

So now we can have a leafletOsmDarkId: "TransportDarkMap" key in layers.yml and if that key exists we construct that layer instead of the normal one and then don't do any filtering.

@hlfan
Copy link
Contributor Author

hlfan commented Dec 18, 2024

Given my limited familiarity with this repo, I decided to place cornerstones in the space of possibilities I saw rather than try for a perfect solution the first few times without much feedback.

But now that I see a more modular solution I'll put this PR on hold too, just fixing the missing filter propagation, and move on to attempt 3 to avoid cluttering up the history.

@hlfan hlfan marked this pull request as draft December 18, 2024 15:09
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.

3 participants