primereact icon indicating copy to clipboard operation
primereact copied to clipboard

Feat: Persist theme configuration across sessions using local storage

Open iamkyrylo opened this issue 1 year ago • 6 comments

Related to a discussion I created recently #2222

Description: The main idea of this PR is to centralize theme configuration changes in one place. I created AppConfigContext component to keep all theme settings, such as theme, dark mode, input style, etc., in one place. It uses the useLocalStorage hook for managing state and storing settings in local storage to make them available across sessions.

To ensure that the brand theme is used for the main landing page, I've chosen to keep two theme values in the state: primeTheme and appTheme.

Here is a demo:

https://github.com/primefaces/primereact/assets/23583235/fe0abf4a-a37f-4cb9-91ea-5875778d474c

Changes:

  • Added AppConfigContext component with a provider to maintain state and change handlers, and the corresponding useAppConfig hook
  • Added missing Jest dependencies to cover all changes with tests and ensure they are safe and function as expected
  • Added test coverage for the Config layout component
  • Improved the config sections accessibility and semantics by providing labels, headers, and aria-roles

Note: Since this is related to the website and from my perspective this is a routine task, I decided to help by providing a pull request for my feature request. It will be totally fine if you decide to decline it.

iamkyrylo avatar Jun 28 '24 16:06 iamkyrylo

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
primereact ⬜️ Ignored (Inspect) Visit Preview Jun 29, 2024 2:53pm
primereact-v9 ⬜️ Ignored (Inspect) Visit Preview Jun 29, 2024 2:53pm

vercel[bot] avatar Jun 28 '24 16:06 vercel[bot]

Just a heads up it used to work this way and PrimeTek intentionally removed it: https://github.com/primefaces/primereact/issues/2671

melloware avatar Jun 28 '24 16:06 melloware

Just a heads up it used to work this way and PrimeTek intentionally removed it: #2671

@melloware, thank you for the heads up. I missed it.

Based on the revert message, I assume it was reverted because we need to call local storage from page components when they are fully loaded by the client, not from the root _app component. This isn't a problem at all. If the overall logic in this PR is okay with you, I can convert the context into a simple useAppConfig hook that will be called by the page components. This is allowed since we use local storage as a state. I considered this option too while working on this PR but initially decided to go with context.

But this is only if I'm not mistaken in my conclusion. I don't see any hydration issues or warnings with this implementation right now, so maybe this will be okay too. Waiting for a review from the team 👀

iamkyrylo avatar Jun 29 '24 15:06 iamkyrylo

Up to you!

melloware avatar Jun 29 '24 15:06 melloware

We are currently working on the new versions of PrimeReact and the PrimeReact Showcase. While we appreciate your suggested change, we have decided not to merge it at this time. However, we liked your approach and will consider this change for our new showcase.

We hope you understand our decision, and we appreciate your continued support. @khris-prog

nitrogenous avatar Jul 03 '24 09:07 nitrogenous

We are currently working on the new versions of PrimeReact and the PrimeReact Showcase. While we appreciate your suggested change, we have decided not to merge it at this time. However, we liked your approach and will consider this change for our new showcase.

We hope you understand our decision, and we appreciate your continued support. @khris-prog

Sure! Not problem at all. I hope this was at least helpful for inspiration🙂

cg-khrishchenko avatar Jul 03 '24 12:07 cg-khrishchenko