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

Remove brightness filter for map tiles in dark mode styles #5327

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sammyhawkrad
Copy link

Description

It's nice to finally have a dark mode on the website but reducing the brightness of the default map tiles is not ideal and negatively affects the user experience. This PR removes the filter on the map tiles to keep the default view of the map.

Sets the dark mode view to this:
image

from this:
image

I also tested the brightness at .95 which is quite bearable if the dimming is necessary.
image

@mxdanger
Copy link
Contributor

Glad to see this change! It's a shame that @AntonKhorev saw all the feedback about the dimmed map and completely ignored it with fast tracking this change #5330 instead of removing the filter all together until #5336 is completed.

@AntonKhorev
Copy link
Collaborator

It's a shame that @AntonKhorev saw all the feedback

I saw that there's a bug.

and completely ignored it with fast tracking this change #5330

This is a fix for that bug.

until #5336 is completed

That's not what necessarily will be deployed.

@Nekzuris
Copy link

What are you waiting to merge this?

@Nekzuris Nekzuris mentioned this pull request Nov 17, 2024
@ChrisJoubert2501
Copy link

What are you waiting to merge this?

This PR needs to be updated, because another PR was recently merged that alters the brightness filter.

@tomFlowee
Copy link

Can someone please rebase and merge this?

Its monday, plenty of office-going people will see the darkened map for the first time and it would help immensely to avoid the influx of bugreports and complaints on support channels.

We know from the large amount of upheaval that this css rule brought that this is not the right solution, so I'm not making this up. We've only seen the volunteer's reaction so far, not the big crowds.

Thanks!

Copy link

1 Warning
⚠️ Merge commits are found in PR. Please rebase to get rid of the merge commits in this PR, see CONTRIBUTING.md.

Generated by 🚫 Danger

@matkoniecz
Copy link
Contributor

What are you waiting to merge this?

Maybe for discussion in #5328 to conclude? Reacting to comments there or posting new, if you have something new to say, may be a good idea.

@mxdanger
Copy link
Contributor

Maybe for discussion in #5328 to conclude?

@matkoniecz The problem is that the map should not have been dimmed to begin with, especially without any proper discussion. This should be merged and the map should remain as default until #5328 is concluded.

@omcnoe
Copy link

omcnoe commented Nov 25, 2024

Maybe for discussion in #5328 to conclude? Reacting to comments there or posting new, if you have something new to say, may be a good idea.

@matkoniecz Whatever the outcome of #5328 comes to it seems very unlikely the consensus will be to keep the current dimming filter as is. This dimming filter is highly controversial and should be reverted until there is consensus on how to approach dark mode for maps.

Opinions elsewhere in the osm community seem pretty upset with the dark mode filter change:
https://community.openstreetmap.org/t/openstreetmap-in-dark-mode/119216/38
https://www.reddit.com/r/openstreetmap/comments/1grclfo/osm_dark_mode_ff_and_chrome

@Nekzuris
Copy link

Nekzuris commented Dec 11, 2024

@gravitystorm I found another technical reason to merge this ASAP: the filter is causing a black and white grid to flicker around tiles borders while zooming on Chrome. It’s slightly noticeable on desktop but very annoying on mobile.
Screenshot_20241207-012830

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

Successfully merging this pull request may close these issues.

8 participants