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

Feat: Persist theme configuration across sessions using local storage #6804

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

iamkyrylo
Copy link
Contributor

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:

primereact.demo.mp4

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.

Copy link

vercel bot commented Jun 28, 2024

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

@melloware
Copy link
Member

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

@iamkyrylo
Copy link
Contributor Author

iamkyrylo commented Jun 29, 2024

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 👀

@melloware
Copy link
Member

Up to you!

@melloware melloware added the Status: Pending Review Issue or pull request is being reviewed by Core Team label Jul 1, 2024
@nitrogenous
Copy link
Contributor

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 nitrogenous added Status: In Progress Core Team is working on the issue or pull request and removed Status: Pending Review Issue or pull request is being reviewed by Core Team labels Jul 3, 2024
@cg-khrishchenko
Copy link

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🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: In Progress Core Team is working on the issue or pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants