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: update terminology and docs #41964

Merged
merged 7 commits into from
Jun 28, 2022

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented Jun 27, 2022

What?

This PR adds a glossary that contains terms and definitions used by the style engine.

It also updates comments and var names so that they're consistent with the definitions.

Part of:

Why?

To ensure consistency and to reflect proper usage.

How?

⌨️

Testing Instructions

Direct link to README.md (so you don't have to parse through HTML tags)

  1. Are the glossary terms accurate and clear?
  2. Are there any typos?
  3. Did I miss anything?
  4. Check the unit tests npm run test-unit-php /var/www/html/wp-content/plugins/gutenberg/packages/style-engine/phpunit/class-wp-style-engine-test.php

Gathering a list of terms.
@ramonjd ramonjd added [Type] Developer Documentation Documentation for developers [Status] In Progress Tracking issues with work in progress [Package] Style Engine /packages/style-engine labels Jun 27, 2022
@ramonjd ramonjd self-assigned this Jun 27, 2022
@ramonjd ramonjd requested review from andrewserong, youknowriad, aaronrobertshaw and glendaviesnz and removed request for andrewserong June 28, 2022 01:45
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.

Nice one, thanks for improving the documentation and adding better clarity to the variable names 👍

It's a bit nit-picky, but I left a couple of comments to see if we need the _array suffix on the variable names. I was wondering if we changed some of the names to be more different to each other, if that'd remove the need for the suffix?

packages/style-engine/class-wp-style-engine.php Outdated Show resolved Hide resolved
packages/style-engine/class-wp-style-engine.php Outdated Show resolved Hide resolved
@ramonjd ramonjd marked this pull request as ready for review June 28, 2022 02:18
@ramonjd ramonjd removed the [Status] In Progress Tracking issues with work in progress label Jun 28, 2022
@ramonjd
Copy link
Member Author

ramonjd commented Jun 28, 2022

Whoops I broke something 😸

Unit tests failing...

Fixing...

ramonjd added 2 commits June 28, 2022 12:38
Cleaning up a few loose ends. Definition should have been declaration :D
@ramonjd
Copy link
Member Author

ramonjd commented Jun 28, 2022

Unit tests failing...

Fixed! T'was a typo.

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 the updates @ramonjd, the changes look good to me, and I smoke tested to make sure it's still working with server-generated styles and individual sides:

image

LGTM 🎉

@ramonjd ramonjd merged commit 24edcdc into trunk Jun 28, 2022
@ramonjd ramonjd deleted the update/style-engine-terminology branch June 28, 2022 05:37
@github-actions github-actions bot added this to the Gutenberg 13.6 milestone Jun 28, 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] Developer Documentation Documentation for developers
Projects
Status: 🏆 Done
Development

Successfully merging this pull request may close these issues.

2 participants