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

Classic themes: Only load classic theme button styles for themes that don't use theme.json #45063

Closed
wants to merge 2 commits into from

Conversation

scruffian
Copy link
Contributor

@scruffian scruffian commented Oct 18, 2022

In #44334 we added fallback styles for buttons to classic themes. This approach won't work for classic themes that use theme.json - the fallback styles should not be for all classic themes, only for those that don't opt in to theme.json.

This changes the conditions when we load this classic theme fallback - now it only loads on themes that don't have theme.json support.

Testing instructions:

  • Switch to Twenty Ten
  • Add some button blocks
  • Add a theme.json file that looks like this:
{
	"version": 2,
    "settings": {
        "layout": {
           "contentSize": "720px",
           "wideSize": "960px"
       }
   },
	"styles": {
		"elements": {
			"button": {
				"color": {
					"background": "yellow",
					"text": "white"
				},
				"border": {
					"color": "white",
					"radius": "70px",
					"style": "solid",
					"width": "4px"
				},
				"typography": {
					"fontWeight": "700"
				}
			}
		},
		"color": {
			"background": "yellow",
			"text": "white"
		}
	}
}
  • Confirm that the button background is still black on trunk in the editor:

Screenshot 2022-10-18 at 10 35 21

- Now switch to this branch and confirm that in the editor the buttons have a yellow background:

Screenshot 2022-10-18 at 10 33 02

cc @WordPress/block-themers

@scruffian scruffian self-assigned this Oct 18, 2022
@scruffian scruffian added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Oct 18, 2022
Copy link
Contributor

@jffng jffng left a comment

Choose a reason for hiding this comment

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

The testing instructions check out, but I'm a bit confused about how the original PR (#44334) is intended to work.

For example, in TT (no theme.json added):

No Gutenberg w/ Gutenberg trunk and this PR
Screen Shot 2022-10-18 at 11 50 27 AM Screen Shot 2022-10-18 at 11 50 10 AM

Why have the button styles changed?

@scruffian scruffian force-pushed the fix/buttons-in-classic-themes branch from ef5277c to 6a3b2e7 Compare October 18, 2022 12:37
@scruffian
Copy link
Contributor Author

Great catch @jffng. I added a commit to address part of that, but I think we also need to fix it in the theme, which I did in the wordpress-develop PR here: WordPress/wordpress-develop#3481

Curious what you think of that approach.

@MaggieCabrera
Copy link
Contributor

We might need to patch more than TTwenty. This is TwentyThirteen:

Screenshot 2022-10-18 at 17 34 49

@scruffian
Copy link
Contributor Author

Twenty Thirteen looks ok to me:
Screenshot 2022-10-18 at 18 04 40

@MaggieCabrera
Copy link
Contributor

Twenty Thirteen looks ok to me

I meant with the theme.json but I guess that's normal, since the theme CSS will override it. It's still working in the background color so it is applying as much as the theme allows

@MaggieCabrera
Copy link
Contributor

I tried a bunch of classic themes and the wordpress-develop PR seems to work as expected

@scruffian
Copy link
Contributor Author

I'm going to hold off merging this until we see what the outcome of WordPress/wordpress-develop#3481 is

@scruffian scruffian force-pushed the fix/buttons-in-classic-themes branch from 9fad82e to 6061e53 Compare March 23, 2023 16:02
@t-hamano
Copy link
Contributor

This PR seems to target WordPress 6.1, but the latest Gutenberg now supports WordPress 6.2 and above. Also, it looks like the backport PR to the core is already committed.

Is it possible to close this PR and #48750?

@scruffian
Copy link
Contributor Author

This has been patched in core so I think we can close it.

@scruffian scruffian closed this Nov 23, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants