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

make theme='inherit' the default #1478

Merged
merged 4 commits into from Aug 9, 2023
Merged

Conversation

mayank99
Copy link
Contributor

@mayank99 mayank99 commented Aug 8, 2023

Changes

changed default value of ThemeProviders theme prop to "inherit" (previously "light").

improved the handling of "inherit" so that it does three things (in order):

  1. try to read theme from context
  2. try to read closest data-iui-theme attribute if no context found
  3. fall back to light theme

as a result, developers will be able to use v3 ThemeProvider inside v2 ThemeProvider and it will automatically work. also, data-iui-theme can be set on the <body> (standalone @itwin/itwinui-variables usage) and it will automatically be respected.

Testing

tested in playground and updated unit tests.

Docs

added changeset.

@mayank99 mayank99 added this to the React 3.0 milestone Aug 8, 2023
@mayank99 mayank99 requested a review from a team as a code owner August 8, 2023 20:40
@mayank99 mayank99 self-assigned this Aug 8, 2023
@mayank99 mayank99 requested a review from a team as a code owner August 8, 2023 20:40
@mayank99 mayank99 requested review from gretanausedaite and rohan-kadkol and removed request for a team August 8, 2023 20:40
@@ -124,6 +133,8 @@ export const ThemeProvider = React.forwardRef((props, ref) => {

export default ThemeProvider;

// ----------------------------------------------------------------------------

const Root = React.forwardRef((props, forwardedRef) => {
Copy link
Member

Choose a reason for hiding this comment

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

nit: Might help to explain what is the purpose of Root, even though it is not exported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was not added in this PR though.

@mayank99 mayank99 merged commit ad62a85 into dev Aug 9, 2023
13 of 15 checks passed
@mayank99 mayank99 deleted the mayank/themeprovider-inherit branch August 9, 2023 16:20
@imodeljs-admin imodeljs-admin mentioned this pull request Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants