fix: change favicon per theme #1348

DiogoSoaress · 2022-12-12T14:42:07Z

What it solves

Resolves #1123

How this PR fixes it

Implements a hook to detect the system's preferred color scheme and subscribes to that media query changes

How to test it

Go to your OS preferences and change the dark/light theme.
Observe the favicon in the browsers tab changing accordingly.

Screenshots

github-actions · 2022-12-12T14:42:35Z

Branch preview

✅ Deploy successful!

https://fixchangefaviconpertheme--webcore.review-web-core.5afe.dev

github-actions · 2022-12-12T14:44:57Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

schmanu · 2022-12-12T15:41:47Z

src/hooks/__tests__/usePrefersColorScheme.test.ts

+import { renderHook } from '@/tests/test-utils'
+
+describe('usePrefersColorScheme', () => {
+  it('should return the initial color scheme', () => {


Could we add more tests for the listeners of the media query which would set the result to dark / light?

Added. I found it a bit difficult to mock the media query event without adding the dependency. Maybe we can look at it quickly together

I can have a look later to do it as described here:
https://jestjs.io/docs/26.x/manual-mocks#mocking-methods-which-are-not-implemented-in-jsdom

src/hooks/usePrefersColorScheme.ts

iamacook · 2022-12-13T10:14:36Z

src/components/common/MetaTags/index.tsx

    <link rel="shortcut icon" href="/favicons/favicon.ico" />
    <link rel="apple-touch-icon" sizes="180x180" href="/favicons/apple-touch-icon.png" />
-    <link rel="icon" type="image/png" sizes="32x32" href="/favicons/favicon-32x32.png" />
-    <link rel="icon" type="image/png" sizes="16x16" href="/favicons/favicon-16x16.png" />
    <link rel="mask-icon" href="/favicons/safari-pinned-tab.svg" color="#000" />


Do we not need to update these other favicons?

I've added dark versions to the ones that were subject to poor contrast in browser's tabs.
If I get it correctly the ones I left in <MetaTags> are for other uses.

I'm still seeing the old icon in Firefox:

I would suggest we test adding an icon to the home screen of an iPhone, as well as pinning a tab in Safari.

I'm still seeing the old icon in Firefox

Must be the cache

I opened it in a Private Window though.

src/hooks/usePrefersColorScheme.ts

iamacook · 2022-12-13T10:19:00Z

src/pages/_app.tsx

+        <link rel="icon" type="image/png" sizes="32x32" href={`/favicons/favicon${darkPath}-32x32.png`} />
+        <link rel="icon" type="image/png" sizes="16x16" href={`/favicons/favicon${darkPath}-16x16.png`} />


Can we not keep these and associated logic inside MetaTag?

I tried to include the logic inside <MetaTag /> component but it was not taking effect. I tried to log from inside that component but unsuccessfully. Happy to look at it together if we want to move these <link> back there

I think it would be best to keep them together. I am happy to look at it with you.

katspaugh

You propose adding 128 lines of JS and an external library to change a favicon.
From the maintainability standpoint, I would much prefer a low-tech solution like an icon with a background.

katspaugh · 2022-12-13T18:36:29Z

src/hooks/usePrefersColorScheme.ts

+    const isDark = window.matchMedia('(prefers-color-scheme: dark)')
+    const isLight = window.matchMedia('(prefers-color-scheme: light)')


Aren't they mutually exclusive?

From my understanding of https://w3c.github.io/csswg-drafts/mediaqueries-5/#prefers-color-scheme they are not mutually exclusive but correct me if I'm wrong. The user can prefer dark or light or have no preference.

Yeah, but you don't have a third icon for !isDark && !isLight, do you?
Also from the docs you linked (emphasis mine):

light
Indicates that user has expressed the preference for a page that has a light theme (dark text on light background), or has not expressed an active preference (and thus should receive the "web default" of a light theme).

katspaugh · 2022-12-13T18:37:21Z

src/pages/_app.tsx

@@ -77,11 +78,16 @@ interface WebCoreAppProps extends AppProps {
 }

 const WebCoreApp = ({ Component, pageProps, emotionCache = clientSideEmotionCache }: WebCoreAppProps): ReactElement => {
+  const prefersColorScheme = usePrefersColorScheme()
+  const darkPath = prefersColorScheme === 'dark' ? '-dark' : ''


darkPath is not the best name if it can be either dark or light.

katspaugh · 2022-12-13T18:42:42Z

There's a CSS-only solution too, have you tried it?
https://stackoverflow.com/a/60158619/352796

DiogoSoaress · 2022-12-14T11:01:19Z

You propose adding 128 lines of JS and an external library to change a favicon.

I know. Well, the changes are more numerous given the added hook and the test coverage. The dev dependency is also only used for tests and it was added following the code review.

I will talk with @liliiaorlenko to get a svg favicon to explore the CSS-only solution and to get her feedback on changing the favicon to a round background that would have good contrast regardless of the OS preferred theme.

DiogoSoaress · 2022-12-14T14:32:09Z

There's a CSS-only solution too, have you tried it?

I was trying this implementation but I don't think it suits our use case give it builds over the assumption that nowadays you can use an SVG as a favicon for your website, which is hardly true.
https://caniuse.com/link-icon-svg

I also got the favicon with the green background from the designer. I will create a different PR for it so we can compare the 2 solutions

DiogoSoaress · 2022-12-15T11:10:57Z

I've addressed the comments and opened an alternative implementation to this PR using the favicon with the green background #1376

katspaugh · 2022-12-15T11:52:24Z

It wasn't necessary to spend time addressing the comments. Closing in favor of the other PR.

DiogoSoaress added 2 commits December 12, 2022 12:05

fix: use preferred color scheme to change favicon

97fb091

implement the hook localy. Test

eb6e89b

DiogoSoaress requested review from katspaugh and schmanu December 12, 2022 14:42

schmanu reviewed Dec 12, 2022

View reviewed changes

DiogoSoaress changed the title ~~Fix change favicon per theme~~ fix: change favicon per theme Dec 12, 2022

tests: add more tests

f298949

DiogoSoaress requested a review from schmanu December 12, 2022 18:22

iamacook reviewed Dec 13, 2022

View reviewed changes

src/hooks/usePrefersColorScheme.ts Show resolved Hide resolved

iamacook reviewed Dec 13, 2022

View reviewed changes

make a generic ColorScheme listener

693c7f2

katspaugh requested changes Dec 13, 2022

View reviewed changes

DiogoSoaress mentioned this pull request Dec 15, 2022

fix: replace favicon by one with a green circle background #1376

Merged

DiogoSoaress added 2 commits December 15, 2022 11:48

rename icons to match preferred color scheme

fdbb774

only listen to changes in prefers-color-scheme: dark media query

c843bbb

katspaugh closed this Dec 15, 2022

katspaugh deleted the fix-change-favicon-per-theme branch December 15, 2022 11:52

github-actions bot locked and limited conversation to collaborators Dec 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: change favicon per theme #1348

fix: change favicon per theme #1348

DiogoSoaress commented Dec 12, 2022

github-actions bot commented Dec 12, 2022 •

edited

Loading

github-actions bot commented Dec 12, 2022 •

edited

Loading

schmanu Dec 12, 2022

DiogoSoaress Dec 12, 2022

schmanu Dec 13, 2022

iamacook Dec 13, 2022

DiogoSoaress Dec 13, 2022

iamacook Dec 13, 2022

DiogoSoaress Dec 13, 2022

iamacook Dec 13, 2022

iamacook Dec 13, 2022

DiogoSoaress Dec 13, 2022 •

edited

Loading

iamacook Dec 13, 2022

katspaugh left a comment

katspaugh Dec 13, 2022

DiogoSoaress Dec 14, 2022

katspaugh Dec 14, 2022

katspaugh Dec 13, 2022

katspaugh commented Dec 13, 2022

DiogoSoaress commented Dec 14, 2022

DiogoSoaress commented Dec 14, 2022 •

edited

Loading

DiogoSoaress commented Dec 15, 2022

katspaugh commented Dec 15, 2022

		<link rel="icon" type="image/png" sizes="32x32" href={`/favicons/favicon${darkPath}-32x32.png`} />
		<link rel="icon" type="image/png" sizes="16x16" href={`/favicons/favicon${darkPath}-16x16.png`} />

		const isDark = window.matchMedia('(prefers-color-scheme: dark)')
		const isLight = window.matchMedia('(prefers-color-scheme: light)')

fix: change favicon per theme #1348

fix: change favicon per theme #1348

Conversation

DiogoSoaress commented Dec 12, 2022

What it solves

How this PR fixes it

How to test it

Screenshots

github-actions bot commented Dec 12, 2022 • edited Loading

Branch preview

github-actions bot commented Dec 12, 2022 • edited Loading

ESLint Summary View Full Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DiogoSoaress Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katspaugh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katspaugh commented Dec 13, 2022

DiogoSoaress commented Dec 14, 2022

DiogoSoaress commented Dec 14, 2022 • edited Loading

DiogoSoaress commented Dec 15, 2022

katspaugh commented Dec 15, 2022

github-actions bot commented Dec 12, 2022 •

edited

Loading

github-actions bot commented Dec 12, 2022 •

edited

Loading

DiogoSoaress Dec 13, 2022 •

edited

Loading

DiogoSoaress commented Dec 14, 2022 •

edited

Loading