Skip to content

Dark mode #21

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions src/components/ColorSchemeToggle.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { useState, useEffect } from 'react';
import Moon from '../icons/Moon';
import Sun from '../icons/Sun';
import { detectColorScheme, switchTheme } from '../util/colorScheme';

const ColorSchemeToggle = () => {
const [dark, setDark] = useState<boolean>();
Copy link
Member

Choose a reason for hiding this comment

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

Should be explicitly "light", "dark" or "system"

Copy link
Member

Choose a reason for hiding this comment

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

This value should also not be managed by a component, instead I suggest using context


useEffect(() => {
setDark(detectColorScheme());
}, []);

const handleClick = (): void => {
setDark((_prev) => !_prev);
Copy link
Member

Choose a reason for hiding this comment

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

Leading with an underscore in variable name is generally used to mark a variable as unused (this is the way VSC deals with it). Also the more explicit previous would be preferred.

switchTheme(dark!);
};

return <button onClick={handleClick}>{dark ? <Sun /> : <Moon />}</button>;
};

export default ColorSchemeToggle;
3 changes: 3 additions & 0 deletions src/components/Navbar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import { useSearch } from '../context/searchContext';
import FilterModal from './FilterModal';
import Search from '../icons/Search';
import Clear from '../icons/Clear';
import { detectColorScheme, switchTheme } from '../util/colorScheme';
Copy link
Member

Choose a reason for hiding this comment

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

Unused imports

import ColorSchemeToggle from './ColorSchemeToggle';

export default function Navbar(): JSX.Element {
const location = useLocation();
Expand Down Expand Up @@ -74,6 +76,7 @@ export default function Navbar(): JSX.Element {
<h1>Songbook</h1>
</div>
<div className="flex-row">
<ColorSchemeToggle />
<button onClick={() => setFilterOpen(true)}>
<Filter />
</button>
Expand Down
14 changes: 14 additions & 0 deletions src/icons/Moon.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import Icon, { IconProps } from './Icon';

export default function Moon({ size, className }: IconProps): JSX.Element {
return (
<Icon size={size} className={className}>
<svg xmlns="http://www.w3.org/2000/svg" width="1em" height="1em" viewBox="0 0 16 16">
<path
d="M6 .278a.768.768 0 0 1 .08.858 7.208 7.208 0 0 0-.878 3.46c0 4.021 3.278 7.277 7.318 7.277.527 0 1.04-.055 1.533-.16a.787.787 0 0 1 .81.316.733.733 0 0 1-.031.893A8.349 8.349 0 0 1 8.344 16C3.734 16 0 12.286 0 7.71 0 4.266 2.114 1.312 5.124.06A.752.752 0 0 1 6 .278z"
fill="currentColor"
/>
</svg>
</Icon>
);
}
14 changes: 14 additions & 0 deletions src/icons/Sun.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import Icon, { IconProps } from './Icon';

export default function Sun({ size, className }: IconProps): JSX.Element {
return (
<Icon size={size} className={className}>
<svg xmlns="http://www.w3.org/2000/svg" width="1em" height="1em" viewBox="0 0 16 16">
<path
d="M8 12a4 4 0 1 0 0-8 4 4 0 0 0 0 8zM8 0a.5.5 0 0 1 .5.5v2a.5.5 0 0 1-1 0v-2A.5.5 0 0 1 8 0zm0 13a.5.5 0 0 1 .5.5v2a.5.5 0 0 1-1 0v-2A.5.5 0 0 1 8 13zm8-5a.5.5 0 0 1-.5.5h-2a.5.5 0 0 1 0-1h2a.5.5 0 0 1 .5.5zM3 8a.5.5 0 0 1-.5.5h-2a.5.5 0 0 1 0-1h2A.5.5 0 0 1 3 8zm10.657-5.657a.5.5 0 0 1 0 .707l-1.414 1.415a.5.5 0 1 1-.707-.708l1.414-1.414a.5.5 0 0 1 .707 0zm-9.193 9.193a.5.5 0 0 1 0 .707L3.05 13.657a.5.5 0 0 1-.707-.707l1.414-1.414a.5.5 0 0 1 .707 0zm9.193 2.121a.5.5 0 0 1-.707 0l-1.414-1.414a.5.5 0 0 1 .707-.707l1.414 1.414a.5.5 0 0 1 0 .707zM4.464 4.465a.5.5 0 0 1-.707 0L2.343 3.05a.5.5 0 1 1 .707-.707l1.414 1.414a.5.5 0 0 1 0 .708z"
fill="currentColor"
/>
</svg>
</Icon>
);
}
27 changes: 19 additions & 8 deletions src/index.scss
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ $main-list-padding: 0.5rem 0 0.75rem;

--background: 255, 255, 255;
--foreground: 0, 0, 0;

--primary: 64, 104, 125;
--primary-medium: 46, 81, 99;
--primary-dark: 28, 42, 51;
Comment on lines 23 to 25
Copy link
Member

Choose a reason for hiding this comment

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

Should also be altered using dark mode to ensure good contrast

Expand All @@ -28,13 +29,18 @@ $main-list-padding: 0.5rem 0 0.75rem;
max-width: 100vw;
}

[data-theme='dark'] {
--background: 28, 42, 51;
--foreground: 255, 255, 255;
}

html,
body,
#root {
margin: 0;
padding: 0;
color: rgba(var(--foreground), 0.85);
background-color: var(--background);
background-color: rgba(var(--background));
height: 100vh;

overflow: hidden;
Expand Down Expand Up @@ -81,7 +87,7 @@ nav {
div.menu {
height: $menu-height;
background-color: rgb(var(--primary-medium));
color: rgba(var(--background), 1);
color: #fff;

@extend .flex-row;
@extend .space-between;
Expand Down Expand Up @@ -127,6 +133,7 @@ nav {
flex: 1;
height: $searchbar-height;
background-color: rgb(var(--background));
color: rgb(var(--foreground));
border: none;
padding: 0.25rem 0.3rem;
font-size: 1.1rem;
Expand Down Expand Up @@ -217,12 +224,16 @@ main.BookmarksList {
.SongListItem {
@extend .list-item;

> li > div {
p {
margin: 0.2rem 0 0 0;
white-space: nowrap;
overflow-x: hidden;
text-overflow: ellipsis;
> li {
background-color: rgba(var(--background));
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be unneeded?


> div {
p {
margin: 0.2rem 0 0 0;
white-space: nowrap;
overflow-x: hidden;
text-overflow: ellipsis;
}
}
}
}
Expand Down
36 changes: 36 additions & 0 deletions src/util/colorScheme.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import React from 'react';

function detectColorScheme(): boolean {
var theme = 'light'; //default to light
Copy link
Member

Choose a reason for hiding this comment

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

Theme should be typed to 'light' | 'dark'.


//local storage is used to override OS theme settings
if (localStorage.getItem('theme')) {
if (localStorage.getItem('theme') == 'dark') {
Comment on lines +7 to +8
Copy link
Member

Choose a reason for hiding this comment

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

Would prefer if the LS key was defined as a const and specified to this application (as seen in bookmarksContext to remove risk of collision when on localhost). E.g. const LS_THEME_KEY = 'in-songbook:theme'

var theme = 'dark';
}
} else if (!window.matchMedia) {
//matchMedia method not supported
return false;
} else if (window.matchMedia('(prefers-color-scheme: dark)').matches) {
Comment on lines +11 to +14
Copy link
Member

Choose a reason for hiding this comment

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

This could be one case window.matchMedia?.('(prefers-color-scheme: dark)')

//OS theme setting detected as dark
var theme = 'dark';
}

//dark theme preferred, set document with a `data-theme` attribute
if (theme == 'dark') {
document.documentElement.setAttribute('data-theme', 'dark');
}
Comment on lines +20 to +22
Copy link
Member

Choose a reason for hiding this comment

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

Since this is only to detect what color scheme to use, this function should not set data-theme

return theme === 'dark';
}

function switchTheme(dark: boolean) {
Copy link
Member

Choose a reason for hiding this comment

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

Should be using explicit strings instead of a boolean.

if (!dark) {
localStorage.setItem('theme', 'dark');
document.documentElement.setAttribute('data-theme', 'dark');
} else {
localStorage.setItem('theme', 'light');
document.documentElement.setAttribute('data-theme', 'light');
}
}

export { detectColorScheme, switchTheme };