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

Switching colors should be done by props, but not components like <LightMode/> and <DarkMode/> #49

Open
whitphx opened this issue Jan 12, 2020 · 1 comment

Comments

@whitphx
Copy link
Contributor

whitphx commented Jan 12, 2020

I think the current way of switching dark/light modes described in https://sancho-ui.com/#theme has a drawback that it forces the child components to re-render
bacause the top-level component changes to the different one, e.g. from LightMode to DarkMode and vice versa.
This React's behavior is described in
https://reactjs.org/docs/reconciliation.html#elements-of-different-types

Whenever the root elements have different types, React will tear down the old tree and build the new tree from scratch.

In most cases, <LightMode /> and <DarkMode /> are the root or near to the root component and re-building its child components takes high cost.
In addition, forcing the children to re-render causes some undesirable/unexpected effects.

Please see the exapmle below.
In the root (App.tsx), the light/dark modes are toggled by changing the root <LightMode /> and <DarkMode /> components.
In the child component (DataFetcher.tsx), it fetches data from an API in async way with useEffect. In this example, the API is dummy and configured to take 1 second for example.

App.tsx

import React, { useState } from 'react';
import { LightMode, DarkMode, Button } from 'sancho';
import DataFetcher from './DataFetcher';

const App: React.FC = () => {
  const [darkMode, setDarkMode] = useState(false);

  const Mode = darkMode ? DarkMode : LightMode

  return (
    <Mode>
      <div>
        <DataFetcher />

        <Button onClick={() => setDarkMode(!darkMode)}>Toggle Dark Mode</Button>
      </div>
    </Mode>
  );
}

export default App;

DataFetcher.tsx

import React, { useState, useEffect } from 'react';

interface DummyData {
  name: string;
}

const dummyAPI = (): Promise<DummyData> => {
  const dummyData: DummyData = {
    name: 'foo'
  };

  return new Promise((resolve) => {
    setTimeout(() => resolve(dummyData), 1000);
  });
}

const DataFetcher = () => {
  const [data, setData] = useState<DummyData>();

  useEffect(() => {
    dummyAPI().then(setData);
  }, []);

  return (
    <div>
      {data && data.name}
    </div>
  );
}

export default DataFetcher;

When you run this code, you will see that each time you toggle the color mode, the fetching occurs and there are 1s delays for the fetched data to be shown:
sancho-rerender-example

This is because, as I wrote above, changing the root component (<LightMode /> and <DarkMode />) forces the child component to re-render and it triggers the fetching in useEffect.

To solve this, I suggest that you provide a single theming component and switching color mode is done by changing its props.
I think ColorMode component defined in https://github.com/bmcmahen/sancho/blob/master/src/Theme/Providers.tsx , which is currently not public is good fit for this purpose.

For example, if the App.tsx is changed like below, the problem is solved.

import React, { useState, useRef } from 'react';
import { useTheme, Theme, ThemeColors, ThemeProvider, Button } from 'sancho';
import DataFetcher from './DataFetcher';

const ColorMode = ...  // ColorMode and its deps are now not public and just copied from the source code.

/**
 * Main app
 */
const App: React.FC = () => {
  const [darkMode, setDarkMode] = useState(false);

  const theme = useTheme();
  const colors = darkMode ? theme.modes.dark : theme.modes.light;

  const colorModeRef = useRef();  // I think this ref should be optional when `ColorMode` is public API.

  return (
    <ColorMode colors={colors} ref={colorModeRef}>
      <div>
        <DataFetcher />

        <Button onClick={() => setDarkMode(!darkMode)}>Toggle Dark Mode</Button>
      </div>
    </ColorMode>
  );
}

export default App;

sancho-no-rerender-example
This shows that unnecessary re-rendering of the child does not occur.

@bmcmahen
Copy link
Owner

bmcmahen commented Jan 14, 2020

This is a great post -- thanks for creating it.

I'm actually surprised that changing the color mode actually causes the component to remount, but it makes sense given your explanation. That's definitely not ideal, and really shouldn't be necessary. I'd like to maintain the current API if possible - so let me explore potential ways to do that, and if I can't figure anything else we can fallback to your solution.

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

No branches or pull requests

2 participants