Skip to content

[Global Styles]: Add REST API endpoint to fetch variations #38124

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.

Sign up for GitHub

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

Jump to bottom
Merged
ntsekouras merged 6 commits into from
Jan 21, 2022
Merged

[Global Styles]: Add REST API endpoint to fetch variations #38124

ntsekouras merged 6 commits into from
Jan 21, 2022

Conversation

@ntsekouras
Copy link
Contributor

@ntsekouras ntsekouras commented Jan 21, 2022

Part of: #35619

In this PR the REST API endpoint is being extracted from the main above PR.

As noted in the original PR about WP_REST_Global_Styles_Controller:

This is just the exact same class that is in Core, the previous version in the Gutenberg plugin was missing some changes that landed directly on Core WordPress/wordpress-develop@9f0d8fd

@ntsekouras ntsekouras added REST API Interaction Related to REST API Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Jan 21, 2022
@ntsekouras ntsekouras self-assigned this Jan 21, 2022
@youknowriad youknowriad self-requested a review January 21, 2022 08:54
@ntsekouras ntsekouras force-pushed the add/global-styles-variations-endpoint branch from 44dba5f to 362774f Compare January 21, 2022 11:05
ntsekouras
ntsekouras commented Jan 21, 2022
View reviewed changes
lib/compat/wordpress-6.0/class-gutenberg-rest-global-styles-controller.php
array(
array(
'methods' => WP_REST_Server::READABLE,
'callback' => array( $this, 'get_theme_items' ),
Copy link
Contributor Author

@ntsekouras ntsekouras Jan 21, 2022

Choose a reason for hiding this comment

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

I think we should rename get_theme_items to get_theme_variations or change the endpoint to be theme-items. This all comes down to whether we want to support more things from this endpoint other than fetching the variations.

Copy link
Member

@Mamaduka Mamaduka Jan 21, 2022

Choose a reason for hiding this comment

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

We will merge this into WP_REST_Global_Styles_Controller in the future, right? In that case using get_theme_variations or get_variation_items make sense.

@ntsekouras
Copy link
Contributor Author

ntsekouras commented Jan 21, 2022

In order to properly test this, I can see three options:

  1. Create the styles folder in the theme and a variation (current implementation, though it seems to have permission issues on the repo that needs to be checked.
  2. Add a new dependency of vfsStream as explained here
  3. Add variations to empty theme in the repo.

I'm not really familiar with php tests, so I could use some feedback.

@Mamaduka
Copy link
Member

Mamaduka commented Jan 21, 2022

Add variations to empty theme in the repo.

It would be helpful for e2e tests as well. We also have two other themes, especially for PHPUnit tests; you can also use them.

@ntsekouras
Copy link
Contributor Author

ntsekouras commented Jan 21, 2022

Add variations to empty theme in the repo.

It would be helpful for e2e tests as well.

Actually this sounds better, yes. We will have e2e tests later on.

ntsekouras
ntsekouras commented Jan 21, 2022
View reviewed changes
lib/compat/wordpress-5.9/class-wp-rest-global-styles-controller.php

if ( rest_is_field_included( 'styles', $fields ) ) {
$raw_data = $theme->get_raw_data();
$data['styles'] = isset( $raw_data['styles'] ) ? $raw_data['styles'] : array();
Copy link
Contributor Author

@ntsekouras ntsekouras Jan 21, 2022

Choose a reason for hiding this comment

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

This in core is $theme->get_raw_data()['styles']; assuming that every theme.json has styles property. We'll need to backport to core. What I'm wondering though should we set this to empty array or not set at all? @oandregal , @jorgefilipecosta

Copy link
Contributor Author

@ntsekouras ntsekouras Jan 25, 2022

Choose a reason for hiding this comment

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

Core patch here: WordPress/wordpress-develop#2224

@ntsekouras ntsekouras marked this pull request as ready for review January 21, 2022 13:36
youknowriad
youknowriad reviewed Jan 21, 2022
View reviewed changes
test/emptytheme/styles/variation.json
@@ -0,0 +1,23 @@
{
Copy link
Contributor

@youknowriad youknowriad Jan 21, 2022

Choose a reason for hiding this comment

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

Nit: I wonder if we should try to make the variation more "meaningful". For instance add a "dark" variation to the empty theme.

Copy link
Contributor Author

@ntsekouras ntsekouras Jan 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I can do it an a follow up to unblock the remaining functionality from landing. I'd appreciate if some theme dev could provide a minimal dark theme.json for empty theme. --cc @kjellr 🙏

Copy link
Contributor

@kjellr kjellr Jan 21, 2022

Choose a reason for hiding this comment

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

Hmm. So this would be just for the emptytheme version bundled here with Gutenberg, right? For the canonical version over in theme-experiments, I don't think adding a style makes sense since it's positioned primarily as an ultra-minimal starting point.

It doesn't even specify a color palette, so making a dark mode version of it is a bit unexpected. 😄

youknowriad
youknowriad approved these changes Jan 21, 2022
View reviewed changes
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ntsekouras ntsekouras merged commit 7da933f into trunk Jan 21, 2022
@ntsekouras ntsekouras deleted the add/global-styles-variations-endpoint branch January 21, 2022 15:51
@github-actions github-actions bot added this to the Gutenberg 12.5 milestone Jan 21, 2022
@ntsekouras ntsekouras mentioned this pull request Jan 25, 2022
Closed
@bph bph added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Mar 24, 2022
@adamziel adamziel mentioned this pull request Apr 4, 2022
Closed
36 tasks
@bph bph mentioned this pull request Apr 5, 2022
Closed
37 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mamaduka Mamaduka Mamaduka left review comments

@youknowriad youknowriad youknowriad approved these changes

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@oandregal oandregal Awaiting requested review from oandregal

@spacedmonkey spacedmonkey Awaiting requested review from spacedmonkey spacedmonkey is a code owner

@TimothyBJacobs TimothyBJacobs Awaiting requested review from TimothyBJacobs

+1 more reviewer

@kjellr kjellr kjellr left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@ntsekouras ntsekouras

Labels

Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json Needs Dev Note Requires a developer note for a major WordPress release cycle REST API Interaction Related to REST API

Projects

None yet

Milestone

Gutenberg 12.5

Development

Successfully merging this pull request may close these issues.

6 participants

@ntsekouras @Mamaduka @youknowriad @kjellr @bph