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

Disable the "Reset styles" button when there are no changes. #63495

Closed
jeflopodev opened this issue Jul 12, 2024 · 5 comments
Closed

Disable the "Reset styles" button when there are no changes. #63495

jeflopodev opened this issue Jul 12, 2024 · 5 comments
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended

Comments

@jeflopodev
Copy link

jeflopodev commented Jul 12, 2024

What problem does this address?

I've just did a styles reset and noticed the "Reset styles" button is still enabled.
image

It let me reset again and It still says there's 1 site change to be saved.
image

Makes zero sense to me. And I find this behavior very confusing for the end user.
It goes against the user to properly identify when the styles are truly reset.
It requires you to remember if you've reset them.

What is your proposed solution?

If the Global Styles are reset. The "Reset styles" button should be disabled.
image

Note that I've used the DevTools to make the text look Greyish (disabled).

@jeflopodev jeflopodev added the [Type] Enhancement A suggestion for improvement. label Jul 12, 2024
@t-hamano
Copy link
Contributor

Thanks for the report.

If the global styles hasn't changed, or if you've reset the global styles once, the button should be disabled. However, for some reason the useGlobalStylesReset hook isn't working properly and canReset always seems to be true.

export const useGlobalStylesReset = () => {
const { user: config, setUserConfig } = useContext( GlobalStylesContext );
const canReset = !! config && ! fastDeepEqual( config, EMPTY_CONFIG );
return [
canReset,
useCallback( () => setUserConfig( EMPTY_CONFIG ), [ setUserConfig ] ),
];
};

The reason for this might be that even if the global styles hasn't changed, the two configs are different, so fastDeepEqual is always false.

config:

{
	settings: {},
	styles: {},
	_links: {
		self: [
			{
				href: 'http://localhost:8888/wp-json/wp/v2/global-styles/5',
			},
		],
		'version-history': [
			// ...
		],
		'wp:action-publish': [
			// ...
		],
		'wp:action-edit-css': [
			// ...
		],
		curies: [],
	},
}

EMPTY_CONFIG:

{
	settings: {},
	styles: {},
}

@t-hamano t-hamano added [Type] Bug An existing feature does not function as intended Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json and removed [Type] Enhancement A suggestion for improvement. labels Jul 13, 2024
@t-hamano
Copy link
Contributor

After some quick investigation, it appears that the newly added _links are involved, as removing the _links from this object resolves the issue:

const config = useMemo( () => {
return {
settings: settings ?? {},
styles: styles ?? {},
_links: _links ?? {},
};
}, [ settings, styles, _links ] );

@t-hamano
Copy link
Contributor

A simple fix would be to rewrite the useGlobalStylesReset hook as follows to exclude everything except settings and styles, but there may be a better way.

diff --git a/packages/block-editor/src/components/global-styles/hooks.js b/packages/block-editor/src/components/global-styles/hooks.js
index 44a3d5e23e..81e57ca5dc 100644
--- a/packages/block-editor/src/components/global-styles/hooks.js
+++ b/packages/block-editor/src/components/global-styles/hooks.js
@@ -85,7 +85,11 @@ const VALID_SETTINGS = [
 ];
 
 export const useGlobalStylesReset = () => {
-       const { user: config, setUserConfig } = useContext( GlobalStylesContext );
+       const { user: _config, setUserConfig } = useContext( GlobalStylesContext );
+       const config = {
+               settings: _config.settings,
+               styles: _config.styles,
+       };
        const canReset = !! config && ! fastDeepEqual( config, EMPTY_CONFIG );
        return [
                canReset,

@andrewserong
Copy link
Contributor

@t-hamano ignoring _links as in your diff above sounds good to me. The _links is essentially metadata so shouldn't be used for the reset comparison.

@t-hamano
Copy link
Contributor

Fixed by #63562

@jeflopodev jeflopodev changed the title Disable the "Reset styles" button, while they are reset, so we can't reset while it's already reset. Disable the "Reset styles" button when there are no changes. Jul 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

3 participants