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

fix: Adds ability to persist theming to local storage. #4367

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

dlabaj
Copy link
Contributor

@dlabaj dlabaj commented Oct 31, 2024

Fixes issue #4365 by persisting theme setting to local storage.

@dlabaj dlabaj added the PF6 label Oct 31, 2024
@dlabaj dlabaj self-assigned this Oct 31, 2024
Copy link
Contributor

@jonkoops jonkoops left a comment

Choose a reason for hiding this comment

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

Might I suggest using the prefers-color-scheme selector to determine the default value? The logic would then essentially be to use the stored value only if it exists, and otherwise fall back to this logic we have Keycloak (but with thepf-v6-theme-dark class).

@patternfly-build
Copy link
Contributor

patternfly-build commented Oct 31, 2024

@dlabaj
Copy link
Contributor Author

dlabaj commented Oct 31, 2024

Might I suggest using the prefers-color-scheme selector to determine the default value? The logic would then essentially be to use the stored value only if it exists, and otherwise fall back to this logic we have Keycloak (but with thepf-v6-theme-dark class).

Thanks for the suggestion will look to see how to use it. Thanks.

@kmcfaul kmcfaul linked an issue Nov 6, 2024 that may be closed by this pull request
Copy link
Contributor

@jonkoops jonkoops left a comment

Choose a reason for hiding this comment

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

Couple of notes.

  1. It would be better to use the storage event instead of visibilitychange, this allows the storage to be synchronized when it changes.
  2. Don't store the preference unless it is done so by user action (e.g. clicking the toggle), instead used the result of the media query. This prevents synchronization issues when the user switches between system preferences.
  3. Use the change event of the media query to dynamically change the dark mode based on system preferences in real time.

You'd basically end up with something like:

const DARK_MODE_CLASS = "pf-v6-theme-dark";
const DARK_MODE_STORAGE_KEY = "dark-mode";
const mediaQuery = window.matchMedia("(prefers-color-scheme: dark)");

// Ensure that the dark mode is correctly set when the page is loaded.
updateDarkMode();

// Update the dark mode when the the user changes the system preference.
mediaQuery.addEventListener("change", () => updateDarkMode());

// Update the dark mode when the user changes the preference in another tab.
window.addEventListener("storage", (event) => {
  if (event.key === DARK_MODE_STORAGE_KEY) {
    updateDarkMode();
  }
});

/**
 * Updates the dark mode based on the stored value (if any) or the system preference.
 */
function updateDarkMode() {
  const storedValue = localStorage.getItem(DARK_MODE_STORAGE_KEY);
  const isEnabled = storedValue === null ? mediaQuery.matches : storedValue === "true";
  const { classList } = document.documentElement;

  if (isEnabled) {
    classList.add(DARK_MODE_CLASS);
  } else {
    classList.remove(DARK_MODE_CLASS);
  }
}

/**
 * Called when the user toggles the dark mode manually through the UI.
 * @param isEnabled Whether the dark mode should be enabled.
 */
function onToggle(isEnabled) {
  localStorage.setItem(DARK_MODE_STORAGE_KEY, isEnabled);
  updateDarkMode();
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug - Dark and light mode are not saved when refreshing the browser
3 participants