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 for confusing palette switch in customizer #4057

Merged
merged 2 commits into from
Aug 16, 2023

Conversation

cristian-ungureanu
Copy link
Contributor

@cristian-ungureanu cristian-ungureanu commented Aug 2, 2023

Summary

This is not exactly a bug as described in the video attached. The problem comes from the fact that we take into consideration the user's last choice inside the customizer. Please see the video for the exact explanation.
What I did here was to actually check if we're inside the customizer and the auto adjust feature is on and if that happens, ignore the user's last pallet choice stored inside the localStorage.

Note for the code reviewer: The script is added in PHP so we minimized it there. Here's the unminimized version of the current changes so you could understand it easier:

/**
 * Get JS contents from file to use as inline script.
 *
 * @return string
 */
public function toggle_script() {
  $auto_adjust = Mods::get($this->get_id() . '_' . self::AUTO_ADJUST, 0);
  $default_state = '"light"';
  
  if ($auto_adjust) {
	  $default_state = 'window.matchMedia && window.matchMedia("(prefers-color-scheme: dark)").matches ? "dark" : "light"';
  }
  
  return '
		    !function() {
		        const e = "neve_user_theme";
		        const t = "data-neve-theme";
  
		        let n = ' . ( ! $auto_adjust && ! is_customize_preview() ? 'localStorage.getItem(e);' : $default_state ) . ';
  
		        document.documentElement.setAttribute(t, n);
  
		        document.addEventListener("click", (n => {
		            if (n.target.matches(".palette-icon-wrapper, .palette-icon-wrapper *")) {
		                (n => {
		                    n.preventDefault();
		                    const a = "light" === document.documentElement.getAttribute(t) ? "dark" : "light";
		                    document.documentElement.setAttribute(t, a);
		                    localStorage.setItem(e, a);
		                })(n);
		            }
		        }));
		    }();
        ';
}

This is the code we currently have on production:

public function toggle_script() {
    $auto_adjust = Mods::get($this->get_id() . '_' . self::AUTO_ADJUST, 0);
    $default_state = '"light"';

    if ($auto_adjust) {
        $default_state = 'window.matchMedia && window.matchMedia("(prefers-color-scheme: dark)").matches ? "dark" : "light"';
    }

    return '
    !function() {
        const e = "neve_user_theme";
        const t = "data-neve-theme";
        
        let n = ' . $default_state . ';
        
        if ("dark" === localStorage.getItem(e)) {
            n = "dark";
        }
        
        document.documentElement.setAttribute(t, n);
        
        document.addEventListener("click", (n => {
            if (n.target.matches(".palette-icon-wrapper, .palette-icon-wrapper *")) {
                (n => {
                    n.preventDefault();
                    const a = "light" === document.documentElement.getAttribute(t) ? "dark" : "light";
                    document.documentElement.setAttribute(t, a);
                    localStorage.setItem(e, a);
                })(n);
            }
        }));
    }();
    ';
}

Will affect visual aspect of the product

NO

Screenshots

https://vertis.d.pr/v/Eu77Sa

Test instructions

  • Import the Agency starter site
  • Go to Customizer and add a palette switcher in the header
  • Inside the palette switcher settings, set a dark palette, other than the "Dark" one
  • Click on the toggle inside the previewer to change the palette so your last choice is the dark one that you've just set
  • Go and change the light palette, the changes should be visible inside the customizer ( until now, they weren't )

Please test the following cases:

  • First, please test that everything remains the same on frontend after you update to this version
  • Please test if the issue is fixed inside the customizer
  • Test that the auto-adjustment feature still works.

Note: This feature already has e2e tests and if the changes I've made affect anything on the front end, they should fail.

Check before Pull Request is ready:

Closes #4025.

@pirate-bot pirate-bot added the pr-checklist-incomplete The Pull Request checklist is incomplete. (automatic label) label Aug 2, 2023
@cristian-ungureanu cristian-ungureanu added the pr-checklist-skip Allow this Pull Request to skip checklist. label Aug 2, 2023
@pirate-bot pirate-bot added pr-checklist-complete The Pull Request checklist is complete. (automatic label) and removed pr-checklist-incomplete The Pull Request checklist is incomplete. (automatic label) labels Aug 2, 2023
@pirate-bot
Copy link
Collaborator

pirate-bot commented Aug 2, 2023

Plugin build for 3fde811 is ready 🛎️!

@cristian-ungureanu cristian-ungureanu linked an issue Aug 2, 2023 that may be closed by this pull request
@irinelenache
Copy link
Contributor

@cristian-ungureanu Tested and everything's fine here 🚀

@cristian-ungureanu cristian-ungureanu merged commit ad58586 into development Aug 16, 2023
47 of 48 checks passed
@cristian-ungureanu cristian-ungureanu deleted the fix/palette-switch branch August 16, 2023 08:58
@pirate-bot
Copy link
Collaborator

🎉 This PR is included in version 3.6.7 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@pirate-bot pirate-bot added the released Indicate that an issue has been resolved and released in a particular version of the product. label Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-checklist-complete The Pull Request checklist is complete. (automatic label) pr-checklist-skip Allow this Pull Request to skip checklist. released Indicate that an issue has been resolved and released in a particular version of the product.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Palette Switch Issue
4 participants