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

feat(theme-default): dynamically change theme-color with dark mode (close #215) #218

Closed
wants to merge 9 commits into from

Conversation

Unbinilium
Copy link

Description

Linked with this feature request issue vuepress/ecosystem#7

Note

For better user experience, the browser should get color hex before the page loaded, but it seems vuepress-next is not currently supported duplicated meta tags like this:

['meta', { name: 'theme-color', content: '#3eaf7c', media: '(prefers-color-scheme: light)' }],
['meta', { name: 'theme-color', content: '#22272e', media: '(prefers-color-scheme: dark)' }]

It means the theme-color was changed after the script executes if system side is dark mode by default, because I could not have different color scheme in duplicated meta tags currently, and due to some reason that #3eaf7c seems too bright that Safari will not use it for address bar color if system side is dark mode by default (darker would works).

Also it may not be the best or proper workaround, some further commit may should be added.

@meteorlxy
Copy link
Member

We are using following snippet to dedupe <meta> tag:

https://github.com/vuepress/vuepress-next/blob/33c9a2145eeaf53429f2ae784e393074d1a9edd5/packages/%40vuepress/shared/src/utils/resolveHeadIdentifier.ts#L11-L14

But I think that supporting multiple <meta name="theme-color"> tags should be another issue.

@Unbinilium
Copy link
Author

Unbinilium commented Jun 14, 2021

We are using following snippet to dedupe <meta> tag:

https://github.com/vuepress/vuepress-next/blob/33c9a2145eeaf53429f2ae784e393074d1a9edd5/packages/%40vuepress/shared/src/utils/resolveHeadIdentifier.ts#L11-L14

But I think that supporting multiple <meta name="theme-color"> tags should be another issue.

https://github.com/vuepress/vuepress-next/blob/b0a9815c9eeaa2ba623cc1a153139e85397d450e/packages/%40vuepress/shared/src/utils/resolveHeadIdentifier.ts#L11-L14

It seems little rude to allow duplicated meta name 'theme-color' in this way, but it simple. Or still avoid duplicated 'theme-color', but keep if they has a different hash.

Copy link
Member

@meteorlxy meteorlxy left a comment

Choose a reason for hiding this comment

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

There's another problem.

Meta tags set in user config will be injected to client with useSiteData. So we also have to update the sitedata ref, or the tag will be reset once navigating to another page.

@meteorlxy
Copy link
Member

The problem about site data could be handled in this PR. If you are not familiar with that, I'll update it later.

@Unbinilium
Copy link
Author

The problem about site data could be handled in this PR. If you are not familiar with that, I'll update it later.

I'm not familiar with that and now I may not have enough time to learn and handle that correctly during my term examination (maybe later I cloud have a try). You can also update it, thanks!

Copy link
Member

@meteorlxy meteorlxy left a comment

Choose a reason for hiding this comment

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

Looks much better.

@meteorlxy
Copy link
Member

Another point:

Currently in this PR, we are using --c-bg and --c-brand as the theme color. But users could set different theme color in head config

@Unbinilium
Copy link
Author

Unbinilium commented Jun 16, 2021

Another point:

Currently in this PR, we are using --c-bg and --c-brand as the theme color. But users could set different theme color in head config

Do you think this workaround would be better? The theme-color now entirely depends on the user's configuration.

let themeColorLight: string
let themeColorDark: string
const updateThemeColor = (): void => {
  const themeColorEl = window?.document.querySelectorAll('meta[name="theme-color"]')
  if (!themeColorEl) return

  // get current system appearance mode by `prefers-color-scheme`
  const systemAppearanceMode = window.matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light'

  // iterate `theme-color` meta tags
  themeColorEl.forEach((meta) => {
    // get `media` attribute from this meta tag
    const mediaAttr = meta.getAttribute('media')

    // initial themeColorLight or themeColorDark if uninitialized by appearance mode
    if (!themeColorLight && mediaAttr?.includes('light')) {
      themeColorLight = meta.getAttribute('content')!
    }
    if (!themeColorDark && mediaAttr?.includes('dark')) {
      themeColorDark = meta.getAttribute('content')!
    }
    
    // if themeColorLight and themeColorDark are initiated, change `content` attribute if the meta `media` attribute matches current system appearance mode
    if (themeColorLight && themeColorLight && mediaAttr?.includes(systemAppearanceMode)) {
      meta.setAttribute('content', isDark.value ? themeColorDark : themeColorLight)
    }
  })
}

But according to #221 (comment), multiple theme-color is currently non-standard in Safari 15, to support the feature described in the title in this way seems more acceptable:

themeConfig: {
  ...
  themeColorLight: '#xxxxxx',
  themeColorDark: '#xxxxxx',
  ....
}

Or we'd better wait new standard came out, thanks your patient advice : )!

@meteorlxy
Copy link
Member

Currently, we could use user config as the light mode theme-color, and use --c-bg as the dark mode theme-color implicitly.

Also, the themeConfig.themeColorLight / themeColorDark could be an alternative.


I've updated this PR with a force push to resolve some conflicts. You could help to check if it works as expected when you have time.

@meteorlxy meteorlxy changed the title feat(theme-default): dynamically change theme-color with toggle-dark-button feat(theme-default): dynamically change theme-color with dark mode (close #215) Jun 17, 2021
@Unbinilium Unbinilium marked this pull request as ready for review June 17, 2021 13:56
@Unbinilium
Copy link
Author

Unbinilium commented Jun 17, 2021

Thanks, it works as expected after fix a logic typo in commit 5bd5498. But has another problem you mentioned before:

There's another problem.

Meta tags set in user config will be injected to client with useSiteData. So we also have to update the sitedata ref, or the tag will be reset once navigating to another page.

And for better dark mode theme-color user experience, some new standard like multiple theme-color tag would be still needed.

Screen.Recording.2021-06-17.at.22.13.27.mov

I changed this PR into draft again, may try to finish this later.

@Unbinilium Unbinilium marked this pull request as draft June 17, 2021 14:34
@Unbinilium Unbinilium marked this pull request as ready for review July 8, 2021 13:53
@Unbinilium
Copy link
Author

Unbinilium commented Jul 8, 2021

Considering that simply changing the meta tag of theme-color in siteData would cause themeColorHeadContent not find the original theme-color after repeatedly switching dark mode, and adding a temporary variable to store the original color would be an inelegant way.

I prefer to do this with a new option in themeConfig, which changes the value of the content of the theme-color meta tag in siteData and current value in <head> via the value of the themeColorLight / themeColorDark options in themeConfig, and a boolean variable called themeColor to control whether or not enable these options.

Tested on my laptop, Safari 15.0:

Untitled.mov

Still have a lot to learn before I can write quality code.

@Unbinilium Unbinilium requested a review from meteorlxy July 10, 2021 17:09
@vercel
Copy link

vercel bot commented Sep 26, 2021

@Unbinilium is attempting to deploy a commit to the VuePress Team on Vercel.

A member of the Team first needs to authorize it.

@jrappen
Copy link

jrappen commented Oct 13, 2021

As a reference, this vuepress 1.x plugin might help:

https://github.com/tolking/vuepress-theme-default-prefers-color-scheme

@meteorlxy
Copy link
Member

Closing as it's stale for a long time. Thanks for all the efforts anyway

@meteorlxy meteorlxy closed this Mar 31, 2023
@meteorlxy
Copy link
Member

Closing as it's stale for a long time. Thanks for all the efforts anyway

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.

3 participants