search

iTwin / iTwinUI-react

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

feat(ThemeProvider): Add `applyBackground` to `themeOptions` #974

Closed mayank99 closed 1 year ago

mayank99 commented 1 year ago

Resolves #967.

What I did

Added an optional boolean applyBackground to themeOptions. When set to true, it will add the iui-root-background class along with iui-root. When false, it will not. The default behavior (when user doesn't pass anything) is to check if theme was already set either through context or on <body> (similar to #963) and add this class if this is the first time setting theme.

To achieve this, I refactored the parent theme check logic into a new hook called useIsThemeAlreadySet. I also had to move the iui-root element above the Provider, and then the code was getting messy so I moved all the complexity into a new internal component called Root. This makes the ThemeProvider component simpler and more performant. (On the note of performance, I also memoized the context value since we are using this context in every component)

Added unit test, and as an additional test that this actually works, I've also updated the vite playground (it was previously using useTheme instead of ThemeProvider).

What this means for the user

In most cases, the user can just do this without even thinking about this new prop:

const App = () => {
  return (
    <ThemeProvider>
      <Button>Hello world</Button>
    </ThemeProvider>
  );
};

createRoot(document.getElementById('root')!).render(<App />);

And if the default behavior is not desirable, they will able to override it easily.

<ThemeProvider themeOptions={{ applyBackground: true }}>
  {/* ... */}
</ThemeProvider>
<ThemeProvider themeOptions={{ applyBackground: false }}>
  {/* ... */}
</ThemeProvider>
mayank99 commented 1 year ago

Update Readme with new functionality. We need better docs for ThemeProvider.

the readme is actually more accurate now than it was previously, because the default behavior will work in most cases. does it make sense to clutter the readme with specific cases? i think the details should go in the documentation site.

i've tried to make the jsdoc very detailed for the applyBackground prop. i can also improve the jsdoc for the ThemeProvider itself.

gretanausedaite commented 1 year ago

the readme is actually more accurate now than it was previously, because the default behavior will work in most cases. does it make sense to clutter the readme with specific cases? i think the details should go in the documentation site.

i've tried to make the jsdoc very detailed for the applyBackground prop. i can also improve the jsdoc for the ThemeProvider itself.

My main idea was to create more docs outside jsdoc. Some people might miss our exposed util components. Jsdoc is not visible unless you add component. This info should be added in website. Until that - where do people find what components we have?

mayank99 commented 1 year ago

My main idea was to create more docs outside jsdoc. Some people might miss our exposed util components. Jsdoc is not visible unless you add component. This info should be added in website. Until that - where do people find what components we have?

I agree jsdoc is not the only place we should document things. That's why we need to prioritize writing more useful docs for our website and deploy it. :)