Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add meta theme color #145

Merged
merged 3 commits into from
Sep 20, 2021
Merged

Conversation

beaufortfrancois
Copy link
Member

This PR adds missing <meta name="theme-color" content="#141216"/> as suggested by Lighthouse.

Issue: #142

@derekherman derekherman requested a review from dero September 15, 2021 06:30
dero
dero previously approved these changes Sep 16, 2021
Copy link
Collaborator

@dero dero left a comment

Choose a reason for hiding this comment

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

This looks good if we want to just appease Lighthouse, approving. 👍

There are, however, some considerations around the dark mode. Right now the dark mode is applied when:

  1. It's enabled in app settings.
  2. It's enabled in the OS (trumps app settings).

While we could do something like this to change the theme-color when dark mode is enabled in the OS:

<meta name="theme-color" content="#fff"/>
<meta name="theme-color" content="#141216" media="(prefers-color-scheme: dark)" />

... we would then still need to dynamically change the former value when dark mode is enabled in the app using settings.

So it seems we might want to set the theme-color dynamically during app initialization based on the --background CSS property:

// Not tested, but something along these lines should work.
const metaThemeColorElement = document.querySelector('meta[name="theme-color"]');
const currentThemeColor = getComputedStyle(document.documentElement).getPropertyValue('--background').trim()

if (currentThemeColor) {
  metaThemeColorElement.setAttribute('content', currentThemeColor);
}

@derekherman derekherman added the bug Something isn't working label Sep 16, 2021
@derekherman
Copy link
Collaborator

It would make sense to fix it for both light and dark mode.

@beaufortfrancois
Copy link
Member Author

Good catch indeed! I've handled light, dark and forced dark mode in latest commits. Please have a look.

Copy link
Collaborator

@dero dero left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@derekherman derekherman merged commit d50e544 into GoogleChrome:develop Sep 20, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants