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.

Sign up for GitHub

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

Jump to bottom

feat(theme): sync body background-color with theme-color meta tag #9325

Merged
merged 4 commits into from
Oct 30, 2023
Merged

feat(theme): sync body background-color with theme-color meta tag #9325

merged 4 commits into from
Oct 30, 2023

Conversation

zica87
Copy link
Contributor

@zica87 zica87 commented Jul 18, 2023

Summary

Fixes #7884.

Problem

(copied from the issue title)

the color of status bar remains white even if dark mode is on. Because of <meta name="theme-color" content="#ffffff" />

Solution

Set "theme-color" to the background color of the page(<body>) on

  • page load
  • user changes theme

Screenshots

Before After
screenshot before screenshot after

How did you test this change?

Tested by running yarn dev and

  • visiting on PC
    • Tried when dark mode on/off and checked that
      • the class of <html> is set correctly
      • the "theme-color" meta tag's content (color)
        • is correct on first visiting the site and
        • changes on changing theme
    • on Vivaldi browser 6.1.3035.111 (based on chromium)
    • win10 22H2 19045
  • visiting on Android phone
    • checked the status bar turns black after changing theme to Dark
    • my phone is too old that doesn't have dark mode so I can't test the color correct or not on first visiting with dark mode turned on
    • on Samsung browser 21.0.3.6 (based on chromium)
    • Android 9

refactor(theme): remove a redundant if block

The if block is identical to line 72-74 and do nothing else.

feat(theme): sync theme-color meta tag with theme

code copied from rust-lang/mdBook#547

fix(theme): properly initialize theme

I guess the useEffect (line 53) is for setting theme on page load, so I think put activeTheme in the dependency list (line 64) is a mistake and thus removed it.

I added ?? "os-default" to properly set "theme-color" meta tag when:

  • the user uses dark mode
  • and either
    • the user didn't select a theme manually (so localStorage.getItem returns null)
    • or localStorage.getItem throws error

Setting theme here has a downside that "theme-color" meta tag will not be set until the content has loaded.
But I think it's OK because:

  • The loading is fast on real developer.mozilla.org. I have never seen the loading screen before testing this PR on local.
  • Users can be hinted by the status bar turning black that "the page has finished loading".
  • The implementation (this PR) is simpler.

@caugner caugner added the 🧑‍🤝‍🧑 community contributions by our wonderful community label Jul 18, 2023
@zica87
Copy link
Contributor Author

zica87 commented Oct 26, 2023

@caugner May I get any response?

@caugner caugner changed the title sync theme-color meta tag with theme feat(theme): sync body background-color with theme-color meta tag with theme Oct 27, 2023
@caugner caugner changed the title feat(theme): sync body background-color with theme-color meta tag with theme feat(theme): sync body background-color with theme-color meta tag Oct 27, 2023
caugner
caugner requested changes Oct 27, 2023
View reviewed changes
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM, just one nit.

client/src/utils.ts Outdated Show resolved Hide resolved
@caugner caugner self-assigned this Oct 27, 2023
@caugner caugner mentioned this pull request Oct 27, 2023
Closed
3 tasks
@zica87 @caugner
fix(theme): check theme-color meta tag and background color exist
b059d30 
Co-authored-by: Claas Augner <495429+caugner@users.noreply.github.com>
@zica87 zica87 requested a review from caugner October 28, 2023 13:53
caugner
caugner approved these changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM, tested locally. 🙌

@caugner caugner enabled auto-merge (squash) October 30, 2023 20:36
@caugner caugner merged commit 5001b04 into mdn:main Oct 30, 2023
10 checks passed
@github-actions github-actions bot mentioned this pull request Oct 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caugner caugner caugner approved these changes

Assignees

@caugner caugner

Labels
🧑‍🤝‍🧑 community contributions by our wonderful community
Projects
None yet
Milestone
No milestone
2 participants
@zica87 @caugner