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

Adds minimum WordPress version to theme metadata #715

Merged
merged 12 commits into from
Sep 20, 2024

Conversation

jffng
Copy link
Contributor

@jffng jffng commented Sep 4, 2024

Description
This PR adds a field to the metadata to allow users to update the required minimum WordPress version in the theme's metadata.

Background
In reviewing some themes recently (Automattic/themes#8058) (Automattic/themes#8067), I've noticed the required minimum version is empty.

I think we should allow the user to be able to edit this via the Metadata editor.

Since this metadata is a requirement, the PR also supplies a minimum version (5.9, when block themes were introduced) if none exists. I'm not sure about this part.

Copy link
Member

@mikachan mikachan left a comment

Choose a reason for hiding this comment

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

Nice, I think this is a good idea to help prevent missing minimum versions.

includes/create-theme/theme-styles.php Outdated Show resolved Hide resolved
includes/create-theme/theme-styles.php Outdated Show resolved Hide resolved
Copy link
Contributor

@matiasbenedetto matiasbenedetto left a comment

Choose a reason for hiding this comment

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

I think this field could be a good candidate for a 'select' type component instead of a plain 'input' one. The select could feature the version '5.9', '6.0', '6.1'.

image

As a probably not-needed addition, each option could have some comment/documentation like "Minimum version for block themes" or something like that.

@richtabor
Copy link
Member

Is there a case where the minimum version would be the version the theme was exported with?

Copy link
Contributor

@matiasbenedetto matiasbenedetto left a comment

Choose a reason for hiding this comment

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

It doesn't seem to be updating the data in the theme as expected. In this screencast I tried updating theme description (working as expected) and WordPress minimum version (not updating).

2024-09-05.11-31-36.mp4

@matiasbenedetto
Copy link
Contributor

Is there a case where the minimum version would be the version the theme was exported with?

That's not always true. A theme could be a very basic template that doesn't require the latest version of Wordpress which is commonly what a theme creator would use.

@jffng
Copy link
Contributor Author

jffng commented Sep 5, 2024

Thanks for the reviews and feedback.

I think this field could be a good candidate for a 'select' type component instead of a plain 'input' one.

I updated the control to be a Select control instead of a text input. An issue with this is that if the theme sets a version not provided by the predetermined options, it won't be reflected in the metadata editor.

Is there a case where the minimum version would be the version the theme was exported with?

It's possible, but do we think that's a better default if no required version is supplied?

It doesn't seem to be updating the data in the theme as expected.

Whoops, I wasn't properly sanitizing the field in the REST API request. Fixed in b1d1558

@richtabor
Copy link
Member

That's not always true. A theme could be a very basic template that doesn't require the latest version of Wordpress which is commonly what a theme creator would use.

Problem is, you don't really know. Picking a version is basically guessing. :)

includes/create-theme/theme-styles.php Outdated Show resolved Hide resolved
Comment on lines 43 to 53
const WP_MINIMUM_VERSIONS = [
'5.9',
'6.0',
'6.1',
'6.2',
'6.3',
'6.4',
'6.5',
'6.6',
'6.7',
];
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems tedious to maintain. How about adding global $wp_version from PHP into a JS global and using that to generate a range of versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@creativecoder do you mean something like this? f3d352a

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's in line with what I was thinking!

@vcanales
Copy link
Member

vcanales commented Sep 12, 2024

do we think that's a better default if no required version is supplied?

I think it would make sense because it would reflect the features available to the theme builder at the time of export.

Copy link
Contributor

@matiasbenedetto matiasbenedetto left a comment

Choose a reason for hiding this comment

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

When you update the minimum version the style.css file is updated but the readme.txt file not.

The minimum version of the blank themes created is hardcoded. Probably we should set something dynamic.

@jffng
Copy link
Contributor Author

jffng commented Sep 17, 2024

Thanks @matiasbenedetto for the review.

When you update the minimum version the style.css file is updated but the readme.txt file not.

Fixed here: 416b149

The minimum version of the blank themes created is hardcoded. Probably we should set something dynamic.

Done here: 8e25b5b

@matiasbenedetto
Copy link
Contributor

The minimum version of the blank themes created is hardcoded. Probably we should set something dynamic.
Done here: 8e25b5b

The default version is empty now when creating a blank theme.

2024-09-18.10-50-12.mp4

@jffng
Copy link
Contributor Author

jffng commented Sep 18, 2024

The default version is empty now when creating a blank theme.

Sorry for not testing that before, I fixed it here: b3b3386. I assumed I could check for the existence of the parameter, but the sanitization function ensures that requires_wp will always exist and falls back to an empty string.

I also added the minimum version selector to the additional metadata panel when creating a new theme: 2f9225f

Here's what I tested:

  • Updating current theme's metadata
  • Creating a blank theme
  • Create a clone theme
  • Creating a child theme

Am I missing any other flows?

Copy link
Member

@mikachan mikachan left a comment

Choose a reason for hiding this comment

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

This is working well in my testing 🎉

I've tested creating themes from the Appearance page, which defaults to using the active WP version in each case:

  • Blank theme ✅
  • Clone ✅
  • Child ✅

I also tested the options available from the Editor, where I was able to set a custom WP version or it would default to the active version:

  • Editing active theme's metadata ✅
  • Blank theme ✅
  • Clone ✅
  • Child ✅

I don't believe there are any other flows and I think this is ready to bring in, so I'm leaving a ✔️ , but it'd be great if @matiasbenedetto has a chance to review before landing too.

@jffng jffng merged commit cdede9a into trunk Sep 20, 2024
2 checks passed
@jffng jffng deleted the add/minimum-version-metadata branch September 20, 2024 16:33
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.

6 participants