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

block.json: Allow passing filename as variations field #6668

Closed

Conversation

ockham
Copy link
Contributor

@ockham ockham commented May 29, 2024

Modeled after https://core.trac.wordpress.org/changeset/54132/, which introduced the render field for block.json, as a way to point to a PHP file instead of providing a render_callback.

The main difference to the variations and variation_callback fields is that the variations field already existed prior to this PR, and it can be used to provide the static list of variations (i.e., an array). This PR makes it so that the field can be alternatively set to a string, which will be interpreted as the filename of the PHP file generating the variations.

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


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.

@ockham ockham self-assigned this May 29, 2024
@ockham ockham changed the title Add variations field to block.json Add variations field to block.json May 29, 2024
@ockham ockham changed the title Add variations field to block.json block.json: Allow passing filename as variations field May 29, 2024
Copy link

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@ockham ockham force-pushed the add/variations-field-to-block-json branch from 94999b4 to f9de198 Compare May 29, 2024 11:03
@ockham ockham force-pushed the add/variations-field-to-block-json branch 2 times, most recently from 041b751 to c10602d Compare June 11, 2024 12:36
@ockham ockham marked this pull request as ready for review June 15, 2024 09:20
Copy link

github-actions bot commented Jun 15, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props bernhard-reiter, gziolo.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@ockham ockham requested review from gziolo and ntsekouras June 15, 2024 09:21
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

That looks like nice enhancement. We will need some changes also on the editor side:

  • extend block.json schema to cover the string value
  • add simple logic that ignores the file path when consuming the metadata from JS as it can’t process PHP file - for WordPress context it’s going to be handled on the server anyway

tests/phpunit/tests/blocks/register.php Outdated Show resolved Hide resolved
@ockham
Copy link
Contributor Author

ockham commented Jun 18, 2024

That looks like nice enhancement. We will need some changes also on the editor side:

  • extend block.json schema to cover the string value

That's handled in WordPress/gutenberg#62092. I'm going to extend that PR to also implement the logic from this PR in the 6.7 compat layer, and to include changes to block variation docs.

  • add simple logic that ignores the file path when consuming the metadata from JS as it can’t process PHP file - for WordPress context it’s going to be handled on the server anyway

👍

@ockham ockham force-pushed the add/variations-field-to-block-json branch from 72a0384 to 5d1eaca Compare June 18, 2024 09:51
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I had one minor comment regarding enforcing file: prefix for paths. However, conceptually everything looks great. I like how this proposal abstracts the usage of lazy evaluated definition of block variations.

It even made me wonder, whether we should do a similar trick with variations_callback for the block variations defined as an array in block.json.

@ockham
Copy link
Contributor Author

ockham commented Jun 18, 2024

I had one minor comment regarding enforcing file: prefix for paths. However, conceptually everything looks great. I like how this proposal abstracts the usage of lazy evaluated definition of block variations.

Thank you! I changed the unit test according to your suggestion 😊 (The code I was using for the variations field was copied from the render field, so it already had a call to remove_block_asset_path_prefix, which processed the file: prefix.)

Since we're still in Beta, I'm planning to wait for RC1 (after which a branch for 6.6 should be created) before I commit this change to trunk.

It even made me wonder, whether we should do a similar trick with variations_callback for the block variations defined as an array in block.json.

You mean so that all variations aren't loaded and built right away if they're specified in block.json? 🤔 Yeah, we could do that. FWIW, I was planning to allow passing a JSON file as variations value, which would allow block authors to simply move a static variations definition to a separate file. This will likely require code that's quite similar.

@gziolo
Copy link
Member

gziolo commented Jun 20, 2024

Since we're still in Beta, I'm planning to wait for RC1 (after which a branch for 6.6 should be created) before I commit this change to trunk.

Yes, it sounds like a great plan.

You mean so that all variations aren't loaded and built right away if they're specified in block.json? 🤔 Yeah, we could do that. FWIW, I was planning to allow passing a JSON file as variations value, which would allow block authors to simply move a static variations definition to a separate file. This will likely require code that's quite similar.

Yes, that would be an enhancement specific only to block.json integration. Referencing a JSON file also sounds like a good addition. The only caveat is that it will need some coordination with WP-CLI team to ensure that translations are still handled for variations when moved to another JSON file.

@ockham
Copy link
Contributor Author

ockham commented Jul 24, 2024

Thank you @gziolo!

I'll have to re-review this PR now that I've landed the GB counterpart, to see if any changes need carrying over.

@ockham ockham force-pushed the add/variations-field-to-block-json branch from 9a30fb2 to e7128b3 Compare July 24, 2024 13:38
@ockham
Copy link
Contributor Author

ockham commented Jul 24, 2024

I'll have to re-review this PR now that I've landed the GB counterpart, to see if any changes need carrying over.

Alright, looks like it doesn't need any changes. I'll land it 😊

@ockham
Copy link
Contributor Author

ockham commented Jul 24, 2024

Committed to Core in https://core.trac.wordpress.org/changeset/58801.

@ockham ockham closed this Jul 24, 2024
@ockham ockham deleted the add/variations-field-to-block-json branch July 24, 2024 14:11
@noisysocks
Copy link
Member

Thanks for committing this early in the 6.7 cycle ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants