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

RFC: Apps Dark Mode Support #7734

Closed
wants to merge 3 commits into from
Closed

RFC: Apps Dark Mode Support #7734

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9f76e84
proof of concept for props / js based dark mode support
georgeblahblah May 11, 2023
8242cdc
refactor to `decidePaletteDark`
georgeblahblah May 11, 2023
c746b1b
remove stale code!
georgeblahblah May 11, 2023
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
12 changes: 11 additions & 1 deletion dotcom-rendering/src/types/palette.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
export type Colour = string;

export type Palette = {
type LightPaletteColours = {
text: {
headline: Colour;
headlineWhenMatch: Colour;
Expand Down Expand Up @@ -150,6 +150,16 @@ export type Palette = {
};
};

export type DarkPaletteColours = {
text: {
headline: Colour;
};
};

export interface Palette extends LightPaletteColours {
dark: DarkPaletteColours;
}

export type ContainerOverrides = {
text: {
cardHeadline: Colour;
Expand Down
28 changes: 28 additions & 0 deletions dotcom-rendering/src/web/components/ArticleHeadline.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import type { SerializedStyles } from '@emotion/react';
import { css } from '@emotion/react';
import type { ArticleFormat } from '@guardian/libs';
import { ArticleDesign, ArticleDisplay, ArticleSpecial } from '@guardian/libs';
Expand All @@ -18,6 +19,27 @@ import { AgeWarning } from './AgeWarning';
import { DesignTag } from './DesignTag';
import { HeadlineByline } from './HeadlineByline';

const darkModeCss =
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this live in its own file?

(supportsDarkMode: boolean) =>
(styles: TemplateStringsArray, ...placeholders: string[]) => {
if (supportsDarkMode) {
const darkStyles = styles
.map(
(style, i) =>
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
`${style}${placeholders[i] ? placeholders[i] : ''}`,
)
.join('');
return css`
@media (prefers-color-scheme: dark) {
${darkStyles}
}
`;
} else {
return '';
}
};

type Props = {
headlineString: string;
format: ArticleFormat;
Expand All @@ -27,6 +49,7 @@ type Props = {
hasStarRating?: boolean;
hasAvatar?: boolean;
isMatch?: boolean;
supportsDarkMode: boolean;
};

const curly = (x: any) => x;
Expand Down Expand Up @@ -342,6 +365,7 @@ export const ArticleHeadline = ({
hasStarRating,
hasAvatar,
isMatch,
supportsDarkMode = false,
}: Props) => {
const palette = decidePalette(format);
switch (format.display) {
Expand Down Expand Up @@ -808,6 +832,10 @@ export const ArticleHeadline = ({
topPadding,
css`
color: ${palette.text.headline};

${darkModeCss(supportsDarkMode)`
color: ${palette.dark.text.headline};
`}
`,
]}
>
Expand Down
3 changes: 3 additions & 0 deletions dotcom-rendering/src/web/layouts/StandardLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,6 +580,9 @@ export const StandardLayout = (props: WebProps | AppProps) => {
hasStarRating={
typeof article.starRating === 'number'
}
supportsDarkMode={
renderingTarget === 'Apps'
}
Comment on lines +583 to +585
Copy link
Contributor

@OllysCoding OllysCoding May 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Rather than deciding this at the layout level, then passing supportsDarkMode to every component, maybe we should only pass renderingTarget around (as this will probably propegate across a lot of the components anyway), then have the darkModeCss function take renderingTarget instead supportsDarkMode and then it becomes the source-of-truth on which rendering targets support dark mode or not (one place)

/>
</div>
</GridItem>
Expand Down
15 changes: 14 additions & 1 deletion dotcom-rendering/src/web/lib/decidePalette.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import {
// Here is the one place where we use `pillarPalette`
import { pillarPalette_DO_NOT_USE as pillarPalette } from '../../lib/pillars';
import type { DCRContainerPalette } from '../../types/front';
import type { Palette } from '../../types/palette';
import type { DarkPaletteColours, Palette } from '../../types/palette';
import { decideContainerOverrides } from './decideContainerOverrides';
import { transparentColour } from './transparentColour';

Expand Down Expand Up @@ -51,6 +51,10 @@ const blogsGrayBackgroundPalette = (format: ArticleFormat): string => {
}
};

const textHeadlineDark = (_format: ArticleFormat): string => {
return 'orangered';
};

const textHeadline = (format: ArticleFormat): string => {
switch (format.display) {
case ArticleDisplay.Immersive:
Expand Down Expand Up @@ -2049,13 +2053,22 @@ const hoverPagination = (format: ArticleFormat) => {
}
};

const decidePaletteDark = (format: ArticleFormat): DarkPaletteColours => {
return {
text: {
headline: textHeadlineDark(format),
},
};
};

export const decidePalette = (
format: ArticleFormat,
containerPalette?: DCRContainerPalette,
): Palette => {
const overrides =
containerPalette && decideContainerOverrides(containerPalette);
return {
dark: decidePaletteDark(format),
text: {
Comment on lines +2071 to 2072
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be a big change but I'd be very open to moving the existing properties into a palette.light, instead of having them at the top level, that way the palette structure is

{
    light: { text: { ... } },
    dark: { text: { ... } }.
}

It'd be a lot of files to change so worth doing in another PR, but I think this would make it really clear that we support both light & dark mode when working on the project, and also lay the groundwork for expansion to support dark mode on the site in the future!

headline: textHeadline(format),
headlineWhenMatch: textHeadlineWhenMatch(format),
Expand Down