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): improve support for respecting system color preferences #16230

Merged
merged 10 commits into from
May 15, 2024

Conversation

lee-chase
Copy link
Member

@lee-chase lee-chase commented Apr 19, 2024

Closes #16222

This PR adds a possible mechanism for use with <Theme> to defer to system color preferences.

This provides for a much simpler mechanism for the user to switch themes based on system settings. It emoves the need for Carbon consumers to implement there own theme classes and media queries to use system settings.

In addition the theme context reports themeCompliment and isDark. The first enables the new <ThemeCompliment> component and the second is extremely useful for those that need to switch colors or images based on a light or dark theme.

Changelog

Changed

  • Modified GlobalTheme/Theme adding a system option to the theme property as well as themeSystemDark and themeSystemLight properties.
  • Updated other stories to report the additional information.
  • Updated the useTheme context to output theme, themeCompliment and isDark.
  • Added a ThemeCompliment component and story.

NOTE: This is as far as I can tell non-breaking.
NOTE 2: If accepted the docs will need updating.

Copy link

netlify bot commented Apr 19, 2024

Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit 9c42df0
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/664501403309c10008f7fea8
😎 Deploy Preview https://deploy-preview-16230--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@lee-chase
Copy link
Member Author

@tay1orjones I think #16222 should be closed in favour of this PR and people encouraged to use the <Theme> component instead of rolling there own CSS.

@lee-chase lee-chase requested a review from a team as a code owner April 19, 2024 17:50
@lee-chase lee-chase requested a review from a team as a code owner May 8, 2024 09:00
@tay1orjones
Copy link
Member

Hey @lee-chase! I met with a couple folks on the team about this yesterday, a few things to note:

  • Theme compliments/inverse should be fully configurable, we don't want to force anything from a design perspective. If someone wants to use white/g10 for light/dark, they can.
  • The theme docs on the website outline a pretty robust framework for how this should work
    • It includes one notable piece - you're allowed to mix inline theme contrast between the different modes (dark on dark for a low-contrast moment, dark on light for a high-contrast moment). So we don't want to force or limit anyone to using a specific inverse/compliment for light or dark.
  • The default theme out of the box is white, while most projects use g10. If we provide a "pair-default", design would prefer the pair-default to be g10/g100. I'm not sure if that makes sense though, we wouldn't want to change the default theme. For now we might want to just have white/g100 as the pair-default, if we bake a pair-default in at all.

What would you think about trying to drop the changes to Theme - themeCompliment, themeSystemDark, themeSystemLight to reduce the internal complexity? Since themeSystemDark, and themeSystemLight can't be limited to two options, I'm not sure that additional complexity justifies itself when isDark gets folks the majority of the way there:

const isDark = useTheme();
...
<GlobalTheme theme={isDark ? "white":"g100"}/>
// Similarly, for configuring a low contrast "dark on dark" local override 
const isDark = useTheme();
...
<Theme theme={isDark ? "g90":"g100"}/>

Am I missing something there? You're much more hands-on familiar with the use case here than I am - it's very possible this doesn't solve the main problem you're running into.

Otherwise just a few follow-on concerns

  • As you've shown, with this approach there's no need to modify the scss and we could instruct other framework authors to recommend or follow this approach.
  • We'd need to document this in storybook and ideally on the website themes code page.
  • Also would be interesting to make a new example or perhaps modify the light-dark-mode example to showcase this with or against the approach there that uses context.

Also thanks again for putting this together, sorry it's taken such time to get a response back on this.

@lee-chase
Copy link
Member Author

@tay1orjones Hmm, food for thought. I do not think I added any restrictions just some conveniences even if perhaps a little verbose.

I like the goal of removing themeCompliment along with themeSystemDark and themeSystemLight but am struggling to see how system preferences are applied while maintaining the current option to ignore user preferences.

I think in the examples you added, isDark is being interpreted as the user preference? If not isDark is being used both to establish context and the theme value which it depends upon.

Perhaps the following would work together, we could even expose a convenient hook.

const userPrefersDark = useUserPrefersDark();
...
<GlobalTheme theme={userPrefersDark ? "g100" : "white"}>
const { isDark } = useTheme();
...
<Theme theme={isDark ? "g100" : "white"} />
<lTheme theme={isDark ? "white" : "g100"}>

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

You pulled together my disparate thoughts so well 😅

I think this looks great! One minor thing I'll add and then LGTM

packages/react/src/components/Theme/index.tsx Show resolved Hide resolved
@tay1orjones tay1orjones removed the request for review from tw15egan May 15, 2024 18:39
@tay1orjones tay1orjones changed the title feat: proposal for system color preferences (v2) feat(theme): improve support for respecting system color preferences May 15, 2024
Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

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

Nice, I like the approach you landed on!

Merged via the queue into carbon-design-system:main with commit 405bc6d May 15, 2024
20 checks passed
@@ -300,7 +300,7 @@ export {
TextDirection as unstable_TextDirection,
} from './components/Text';
export { DefinitionTooltip } from './components/Tooltip/DefinitionTooltip';
export { GlobalTheme, Theme, useTheme } from './components/Theme';
Copy link
Member Author

Choose a reason for hiding this comment

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

Was the export of useTheme @tay1orjones accidentally removed

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.

None yet

3 participants