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 main public function to reflect functionality #42140

Merged
merged 3 commits into from
Jul 6, 2022

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented Jul 5, 2022

What?

Renames wp_style_engine_generate to wp_style_engine_generate_block_styles to better describe the first (and required) argument: a single block style object.

I also took the opportunity to rename some references from styles to declarations for clarity.

Why?

One day we'll likely one to pass the theme.json styles object and return a stylesheet. Something like wp_style_engine_generate_global_styles

Or not. Whatever.

This change ensures we won't be stuck with a generic name for a function that specifically deals in block styles.

How?

With a keyboard.

Testing Instructions

Run the tests!

npm run test-unit-php /var/www/html/wp-content/plugins/gutenberg/packages/style-engine/phpunit/class-wp-style-engine-test.php

npm run test-unit-php

@ramonjd ramonjd added [Type] Code Quality Issues or PRs that relate to code quality [Package] Style Engine /packages/style-engine labels Jul 5, 2022
@ramonjd ramonjd requested review from aristath and andrewserong July 5, 2022 01:16
@ramonjd ramonjd requested a review from spacedmonkey as a code owner July 5, 2022 01:16
@ramonjd ramonjd self-assigned this Jul 5, 2022
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.

This rename looks good to me @ramonjd, and tested that the border, colors, elements, spacing, and typography supports still work for server-rendered blocks 👍

One potential issue with the naming is that gutenberg_style_engine_generate_block_styles shares the name block styles with the block styles feature, where this function generates the styles for block supports, which is semantically slightly different, but tricky to neatly fit into a name!

I don't feel too strongly about the naming, though, and I agree with the premise behind this PR, which is that it'd be better for the public function name to be a little more specific to this particular use case of the style engine, so that we're freed up to explore calling the style engine in a different way if need be for when we tackle global styles.

So, not a blocker from me if you (or anyone else) think we should just stick with block_styles for now 🙂

@ramonjd
Copy link
Member Author

ramonjd commented Jul 5, 2022

One potential issue with the naming is that gutenberg_style_engine_generate_block_styles shares the name block styles with the block styles feature, where this function generates the styles for block supports, which is semantically slightly different, but tricky to neatly fit into a name!

This is a great point @andrewserong, thanks for raising it.

I was hoping that the style-engine namespace would provide context, but it is an unfortunate similarity. "Block styles" is also a bit vague, where I think it's rather something more like "Block style/appearance variations"

In my mind, I consider block styles (for the purposes of the style engine) to mean not only the object that blocks have in attributes.style, but all the objects that can be under styles.elements.h1 or styles.blocks['core/button'] in global styles for example.

Proof that this definition works is over in #42143.

So maybe we need another term to cover that "styles" object.

Possibly just wp_style_engine_generate_styles.

I'm very open to add block_supports to the method names.

It's enough that we differentiate the methods from whatever use for global styles later, e.g., wp_style_engine_generate_global_stylesheet

One thing to note is that I'm experimenting with splitting getting classnames from getting CSS, so wp_style_engine_generate_css and wp_style_engine_generate_classnames.

@andrewserong
Copy link
Contributor

andrewserong commented Jul 5, 2022

Thanks for the extra detail @ramonjd! Would wp_style_engine_generate_block_supports_styles be too much of a mouthful? (Again, I don't feel too attached to which particular name we go for 😄)

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

I've given this a run as well.

✅ Unit tests pass
✅ Border, color, spacing, and typography supports still work for dynamic blocks
✅ Couldn't see any missed references to gutenberg_style_engine_generate
✅ Switching references from styles to declarations makes sense

I had similar thoughts as @andrewserong regarding the overlap between the new naming here with block styles and the concept of Block Styles in the Block API. I wouldn't be opposed to making the new name slightly more verbose to distinguish the two more. Certainly not a blocker for me either though.

@ramonjd
Copy link
Member Author

ramonjd commented Jul 5, 2022

Thanks for your input @aaronrobertshaw and @andrewserong

Since you both had the same idea, I'll err on the side of verbosity to avoid confusion.

I wouldn't be opposed to making the new name slightly more verbose to distinguish the two more.
Would wp_style_engine_generate_block_supports_styles be too much of a mouthful?

wp_style_engine_get_block_supports_styles would be one more character than wp_enqueue_editor_block_directory_assets 🤣

@aristath
Copy link
Member

aristath commented Jul 5, 2022

+1 for block_supports_styles

ramonjd added 3 commits July 6, 2022 09:20
…yles to better describe the first (and required) argument: a single block style object.

Renaming references from styles to declarations for clarity.
Adding PHP doc comments to tests.
…k styles, which is an existing paradigm that pertains to "style variations".
@ramonjd ramonjd force-pushed the update/style-engine-rename-method-and-properties branch from e515464 to d1532cc Compare July 5, 2022 23:20
@ramonjd ramonjd merged commit 0933055 into trunk Jul 6, 2022
@ramonjd ramonjd deleted the update/style-engine-rename-method-and-properties branch July 6, 2022 00:24
@github-actions github-actions bot added this to the Gutenberg 13.7 milestone Jul 6, 2022
@ramonjd ramonjd changed the title Style engine: rename public method Style engine: rename main public function to reflect functionality Jul 6, 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] Code Quality Issues or PRs that relate to code quality
Projects
Status: 🏆 Done
Development

Successfully merging this pull request may close these issues.

4 participants