search

use-ink / contracts-ui

Web application for deploying wasm smart contracts on Substrate chains that include the FRAME contracts pallet
https://contracts-ui.substrate.io/
GNU General Public License v3.0
61 stars 44 forks source link

(discussion) All the app re-renders when the users change the theme #477

Closed NoriSte closed 1 year ago

NoriSte commented 1 year ago

I was asked to analyze the current application state management and optimization of re-renders. This issue is meant to be a discussion around the topic instead of a "let's do that now" solution. The reason why I want to discuss the issue before implementing it is because:

  1. I miss the context: this is my first contribution to the project and I could completely miss the context of the real uses cases and problems.

  2. This is a static analysis: the issue I want to discuss does not come from analyzing complex use cases nor from some users reporting a problem.

  3. The current situation offers a great DX: I will discuss it later, but the current solution offers a great UX even if rerendering the whole app could sound suboptimal.

Current situation

The whole light/dark theme is based on the Tailwind-related class (see Toggling dark mode manually and https://github.com/paritytech/contracts-ui/blob/66a276e314ee5c2da85277b7a3eaed7a9667f9aa/tailwind.config.cjs#L6 ) and the ThemeContextProvider is used at the top of the application tree https://github.com/paritytech/contracts-ui/blob/66a276e314ee5c2da85277b7a3eaed7a9667f9aa/src/ui/components/App.tsx#L15.

The theme can be switched by simply toggling the dark class in the DOM, but the current ThemeContextProvider also forces the whole component tree to re-render.

To Reproduce Steps to reproduce the behavior:

  1. Install the React DevTools in your browser
  2. Launch the Contracts UI
  3. Open the Contracts UI's Settings
  4. Open the React DevTools
  5. Enable highlighting the component re-renders (see screenshot) image
  6. In the Contract UI, toggle the theme between light and dark

As you can see, all the application is highlighted (with a cyan border) since everything is re-rendered.

(if you want to check it online, you can use one of the "Deploy Preview" of a PR, like the #476 one)

Possible solution

Move all the theme management directly to the ThemeMode component https://github.com/paritytech/contracts-ui/blob/66a276e314ee5c2da85277b7a3eaed7a9667f9aa/src/ui/components/settings/ThemeMode.tsx#L19 that's the only current direct consumer of the theme, and removing the ThemeProvider.

You can check that this would work since to switch the light/dark theme you only need to add/remove the dark class from the html DOM element, no need for any React Context around the whole application.

image

Devil's advocate

Why the optimization proposed in this issue should not be implemented?

  1. It could not be a problem at all: as I said at the beginning, this issue is not the result of analyzing a problem, but more the result of an external and static analysis.

  2. The current UX is good: even if the whole render tree rerenders when the theme changes, and even if this action would take (for instance) 1 second (that's a lot), it is the result of an explicit users' action. That means that the users expect that something changes (and potentially requires some time) when they directly change something that impacts the whole application!

  3. The current ThemeProvider encapsulates the theme management: this eases the future refactor, and that's a really good thing! If in six months we need to migrate the theme management away from Tailwind, we only need to change the ThemeProvider and everything propagates to the whole application.

  4. Reactivity to its best: the current ThemeProvider not only encapsulates all the theme management but also ensures the component tree is re-rendered when something with such a broad scope (the theme impacts everything) changes. Again, if the theme will be managed in a different way in the future, this is great!

  5. Simulating themes is easy: If in the future we need to write component tests in Vitest/Cypress or stories in Storybook, etc. the ThemeProvider allows simulating all the theme options in a while, offering the best possible DX.

Conclusion

My two cents: I would keep the ThemeProvider as is, but I'd like to know what you think about the topic 😊

Notes

I did not use any of the existing templates because this issue is not a Bug report, nor a Feature request, nor a Security vulnerability.

NoriSte commented 1 year ago

(I opened this issue by mistake, I will update and reopen it as soon as it's ready)

NoriSte commented 1 year ago

The issue is now ready to be discussed, I reopen it 😊