Skip to content

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 requested review from evwilkin and kmcfaul October 31, 2024 20:15
@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
Collaborator

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