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

dispatch theme changes via custom events #1731

Merged
merged 6 commits into from
Dec 12, 2023
Merged

Conversation

mayank99
Copy link
Contributor

@mayank99 mayank99 commented Dec 8, 2023

Changes

Context (no pun intended): In #1478, we made theme="inherit" more resilient, by reading the data-iui-theme attribute when react Context fails (for example, v2 Context cannot be consumed in v3). However this only works on first load. Subsequent theme changes are not reflected.

This PR: The ThemeProvider root element will now dispatch native custom events which can be subscribed to anywhere in the tree regardless of Context or package version. Specifically, iui:themechange events will be dispatched every time the theme prop changes. Then, for the theme="inherit" fallback, we will listen to these events and update the theme each time.

Subsequent PR: The same change will need to be made in v2, so that v3 can listen to theme changes from v2.

Testing

A little difficult to test locally without making a release, but it can be emulated in vite playground by manually dispatching events (pretend these events are dispatched by v2 ThemeProvider).

main.tsx
import * as React from 'react';
import { createRoot } from 'react-dom/client';
import styled from '@emotion/styled';
import { ThemeProvider } from '@itwin/itwinui-react';
import App from './App.tsx';
import { SvgMoon, SvgSun } from '@itwin/itwinui-icons-react';
import '@itwin/itwinui-react/styles.css';

const Shell = () => {
  const rootRef = React.useRef<HTMLDivElement>(null);
  const [theme, setTheme] = React.useState<'light' | 'dark'>(() =>
    matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light',
  );
  const [highContrast, setHighContrast] = React.useState(false);

  return (
    <>
      <div data-iui-theme={theme} ref={rootRef}>
        <Main>
          <ThemeToggle
            aria-label='Toggle theme'
            onClick={() => {
              const newThemeValue = theme === 'dark' ? 'light' : 'dark';
              setTheme(newThemeValue);
              rootRef.current?.dispatchEvent(
                new CustomEvent('iui:themechange', {
                  detail: { theme: newThemeValue, highContrast },
                }),
              );
            }}
          >
            {theme === 'dark' ? <SvgMoon /> : <SvgSun />}
          </ThemeToggle>

          <HighContrastToggle
            onClick={() => {
              const newHcValue = !highContrast;
              setHighContrast(newHcValue);
              rootRef.current?.dispatchEvent(
                new CustomEvent('iui:themechange', {
                  detail: { theme, highContrast: newHcValue },
                }),
              );
            }}
          >
            {highContrast ? 'High Contrast' : 'Regular contrast'}
          </HighContrastToggle>

          <ThemeProvider theme='inherit'>
            <App />
          </ThemeProvider>
        </Main>
      </div>
    </>
  );
};

const Main = styled.main`
  padding: 2rem 1rem;
  height: 100vh;
`;

const ThemeToggle = styled.button`
  all: unset;
  display: inline-grid;
  place-items: center;
  position: fixed;
  font-family: system-ui;
  user-select: none;
  top: 0;
  right: 0;
  min-width: 2.5rem;
  height: 2.5rem;
  cursor: pointer;
  border-radius: 50%;

  &:hover {
    background: hsl(0 0% 0% / 0.2);
  }

  & > * {
    width: 1.5rem;
    height: 1.5rem;
    fill: currentColor;
  }
`;

const HighContrastToggle = styled(ThemeToggle)`
  right: 2.5rem;
`;

createRoot(document.getElementById('root')!).render(
  <React.StrictMode>
    <Shell />
  </React.StrictMode>,
);

Added unit tests as well.

After this PR is merged, I will backport it to v2, make a release, then test with actual v2 ThemeProvider.

Docs

N/A

@mayank99 mayank99 self-assigned this Dec 8, 2023
@mayank99 mayank99 linked an issue Dec 8, 2023 that may be closed by this pull request
@mayank99 mayank99 marked this pull request as ready for review December 11, 2023 20:46
@mayank99 mayank99 requested review from a team as code owners December 11, 2023 20:46
@mayank99 mayank99 requested review from r100-stack and removed request for a team December 11, 2023 20:46
Copy link
Member

@r100-stack r100-stack left a comment

Choose a reason for hiding this comment

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

I left a few nit comments in ThemeProvider.tsx. They were mainly for me to better understand the code. So, feel free to ignore those suggestions if the code is self-explanatory as-is and also maybe also correct me if I misunderstood what the highlighted code does.

@mayank99 mayank99 merged commit eb429a0 into main Dec 12, 2023
16 checks passed
@mayank99 mayank99 deleted the mayank/dispatch-theme-changes branch December 12, 2023 15:33
@imodeljs-admin imodeljs-admin mentioned this pull request Dec 12, 2023
@raplemie
Copy link
Contributor

@mayank99, since this will require the v2 package to be up to date for the event to be fired, and you need a ref to the element anyway, would MutationObserver allow the same thing but would not require the new event ? (By looking at mutation on the ref attributes, which I think contain all the information needed (theme and high contrast))

@mayank99
Copy link
Contributor Author

@mayank99, since this will require the v2 package to be up to date for the event to be fired, and you need a ref to the element anyway, would MutationObserver allow the same thing but would not require the new event ? (By looking at mutation on the ref attributes, which I think contain all the information needed (theme and high contrast))

@raplemie i find mutation observers to be too unwiedly. events feel more natural because of their "dispatch"/"subscribe" model. however, considering how bad applications are at consuming the latest version (many of them are still on 2.11.x), it is the better option overall. i will create another PR, thanks for the suggestion.

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.

V2/V3 Theme synchronisation
3 participants