-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat(theme): improve support for respecting system color preferences #16230
Conversation
✅ Deploy Preview for v11-carbon-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@tay1orjones I think #16222 should be closed in favour of this PR and people encouraged to use the |
c0a0c3e
to
73b986f
Compare
Hey @lee-chase! I met with a couple folks on the team about this yesterday, a few things to note:
What would you think about trying to drop the changes to Theme - 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
Also thanks again for putting this together, sorry it's taken such time to get a response back on this. |
@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 I think in the examples you added, Perhaps the following would work together, we could even expose a convenient hook.
|
There was a problem hiding this 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
There was a problem hiding this 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!
405bc6d
@@ -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'; |
There was a problem hiding this comment.
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
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
andisDark
. 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
system
option to the theme property as well asthemeSystemDark
andthemeSystemLight
properties.useTheme
context to output theme, themeCompliment and isDark.NOTE: This is as far as I can tell non-breaking.
NOTE 2: If accepted the docs will need updating.