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

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

Conversation

andrewserong
Copy link
Contributor

@andrewserong andrewserong commented Sep 19, 2022

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
Contributor Author

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?

@scruffian
Copy link

Should we also add it to remove_insecure_properties?

@andrewserong
Copy link
Contributor Author

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 👍

Copy link
Contributor

@cbravobernal cbravobernal left a comment

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
Contributor

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
Contributor

desrosj commented Sep 20, 2022

@desrosj desrosj closed this Sep 20, 2022
@desrosj
Copy link
Contributor

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
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

4 participants