Skip to content

Backport box-shadow support for blocks via theme.json#3274

Closed
andrewserong wants to merge 1 commit intoWordPress:trunkfrom
andrewserong:add/shadow-support-to-theme-json
Closed

Backport box-shadow support for blocks via theme.json#3274
andrewserong wants to merge 1 commit intoWordPress:trunkfrom
andrewserong:add/shadow-support-to-theme-json

Conversation

@andrewserong
Copy link
Copy Markdown
Contributor

@andrewserong andrewserong commented Sep 19, 2022 •
edited
Loading

This PR backports the following Gutenberg PR: WordPress/gutenberg#41972. Note that box-shadow is already listed in PROPERTIES_METADATA so that line has already been backported.

See tracking issue: WordPress/gutenberg#43440

Trac ticket: https://core.trac.wordpress.org/ticket/56467

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@andrewserong
Copy link
Copy Markdown
Contributor Author

andrewserong commented Sep 19, 2022

CC: @madhusudhand and @scruffian, I was looking through the Theme JSON class, and it looked like this one hadn't been backported yet, and I assume it's intended to make it into 6.1?

@andrewserong andrewserong mentioned this pull request Sep 19, 2022
Closed
37 tasks
@scruffian
Copy link
Copy Markdown
Contributor

scruffian commented Sep 19, 2022

Should we also add it to remove_insecure_properties?

@andrewserong
Copy link
Copy Markdown
Contributor Author

andrewserong commented Sep 20, 2022

Thanks for taking a look @scruffian! I double checked, and it looks like we don't need to manually specify it in remove_insecure_properties because when remove_insecure_styles is called, it'll do the lookup in compute_style_properties to static::PROPERTIES_METADATA which already includes the box-shadow key. So, I believe it's already handled 👍

cbravobernal
cbravobernal approved these changes Sep 20, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@cbravobernal cbravobernal left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM - Tested and works as expected!

@desrosj desrosj self-assigned this Sep 20, 2022
@desrosj
Copy link
Copy Markdown
Member

desrosj commented Sep 20, 2022

Is there any way that we can write some unit tests for this? I feel like we have not kept up with the new settings and styles allowed in theme.json. Our test theme is missing quite a few of the items listed in the reference.

I think this fine to merge in as is, but I'd like to open a ticket to expand our theme.json testing.

@desrosj
Copy link
Copy Markdown
Member

desrosj commented Sep 20, 2022

Merged in https://core.trac.wordpress.org/changeset/54253.

@desrosj desrosj closed this Sep 20, 2022
@desrosj
Copy link
Copy Markdown
Member

desrosj commented Sep 20, 2022

Created Core-56611 for expanding unit tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cbravobernal cbravobernal cbravobernal approved these changes

Assignees

@desrosj desrosj

Labels

None yet

Projects

No open projects
WordPress 6.1 Editor Tasks
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@andrewserong @scruffian @desrosj @cbravobernal