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

Style engine: rename global function #42719

Merged
merged 1 commit into from
Jul 27, 2022

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented Jul 27, 2022

What?

Rename wp_style_engine_get_block_supports_styles to wp_style_engine_get_styles and document a 'context' option that will determine how we treat incoming style object.

Why?

We'll potentially have multiple origins of styles, even though those style object will be similar. For example, the value of a block's attributes.style object or the top level styles in theme.json.

To avoid having multiple, specifically-named global functions, why not one that can handle everything?

Better for future compatibility, flexibility and better for our brains!

Props to @aristath for the idea.

How?

Renaming.

Testing Instructions

Run npm test-unit-php

Smoke test a site to ensure nothing breaks.

…et_styles and document a 'context' option that will determine how we treat incoming style object.
@ramonjd ramonjd added [Type] Enhancement A suggestion for improvement. [Package] Style Engine /packages/style-engine labels Jul 27, 2022
@ramonjd ramonjd requested review from aristath and andrewserong July 27, 2022 05:44
@ramonjd ramonjd requested a review from spacedmonkey as a code owner July 27, 2022 05:44
@ramonjd ramonjd self-assigned this Jul 27, 2022
Copy link
Member

@aristath aristath left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Right now there's only block-supports, but in the future I assume we'll need to add a $context arg in the function to differentiate between global-styles, block-supports and any other implementations we may have.
For now since block-supports is the only one, just renaming the function will be enough 👍

@ramonjd
Copy link
Member Author

ramonjd commented Jul 27, 2022

I assume we'll need to add a $context arg in the function to differentiate between global-styles, block-supports and any other implementations we may have.

Yeah, I documented it in the PHP doc but assumed that 'block-supports' would be default for now.

Thank you for looking at this 🍺

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

Thanks for putting this up, and +1 thanks for the suggestion @aristath!

✅ Rename looks good
✅ Smoke tested the block supports still work correctly on the site frontend

Right now there's only block-supports, but in the future I assume we'll need to add a $context arg in the function to differentiate between global-styles, block-supports and any other implementations we may have.

Agreed — since the implementation doesn't yet enqueue the styles and use the store, this PR as-is looks good to merge to me, and we can add the $context arg once we start enqueuing as in #42452?

LGTM!

@ramonjd ramonjd merged commit d0f881c into trunk Jul 27, 2022
@ramonjd ramonjd deleted the update/style-engine-rename-global-function branch July 27, 2022 23:32
@github-actions github-actions bot added this to the Gutenberg 13.9 milestone Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Style Engine /packages/style-engine [Type] Enhancement A suggestion for improvement.
Projects
Status: 🏆 Done
Development

Successfully merging this pull request may close these issues.

3 participants