search

iTwin / iTwinUI-react

A react component library for iTwinUI.
https://github.com/iTwin/iTwinUI
Other
83 stars 23 forks source link

ThemeProvider removes background color from body #967

Closed Woyken closed 1 year ago

Woyken commented 1 year ago

Describe the bug (actual behavior)

When ThemeProvider is used in from react SPA, body will loose 'iui-root' class

Expected behavior

React rendered website still contains background

Reproduction

https://codesandbox.io/s/itwinui-react-example-forked-ohl4nl?file=/src/index.tsx

Steps to reproduce
  1. Render SPA react app
  2. Use ThemeProvider
  3. Background gone

image

mayank99 commented 1 year ago

It is not "removing" the body background so much as never adding it. There are two things going on here:

  1. <ThemeProvider> is what is being rendered, so if you don't render it in place of <body> then it has no reason to touch <body>.
  2. Our css intentionally doesn't set a background on iui-root if it is not used on <body>. This is because you might want to created scoped roots that only toggle the theme without any side effects.

Are you able to use the <html> element as the react root and render

ReactDOMClient.createRoot(document.documentElement).render(
  <React.StrictMode>
    <ThemeProvider theme='dark' as='body'>
      <App />
    </ThemeProvider>
  </React.StrictMode>
)
veekeys commented 1 year ago

@mayank99 It does not make sense. If you just use iTwinUI components (do not use ThemeProvider), the styles ARE set on body and everything works fine. Now if you use ThemeProvider in your app, then those are NOT set. It is not obvious behavior and I do not think its explainable. And default setup of Vite, CRA, etc is index.html and all the setup there. So now just to make theming of iTwinUI work, you need to change all of these things.

mayank99 commented 1 year ago

Maybe we can add a prop that sets the background if ThemeProvider is used as a child of body?

veekeys commented 1 year ago

I think there is just 2 different cases. One is literally managing the theme of the app (usual case and in v1 using useTheme was really comfortable and nice, IMO). So this one should be simple and should not need some advanced configuration. And rarer case (at least currently) is scoping themes.

So with v2, 2nd one is really nice, API is super and comfortable. But now the 1st one is not that obvious and more complex :/

Woyken commented 1 year ago

Usually SPA react app is rendered on a \

So it won't always be a child of body.

I think it would make sense if ThemeProvider had prop called "applyBackground" or something similar. That would set the background-color css on the div or whatever the underlying element is.

mayank99 commented 1 year ago

@Woyken Forgot to reply here but this is now fixed/added in 2.1.0. It should work as expected right out of the box, and you can also override the default behavior by passing themeOptions.applyBackground. See the PR description in #974 for more details.