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

Remove the experiment option for Block Theme Previews #50983

Conversation

okmttdhr
Copy link
Contributor

@okmttdhr okmttdhr commented May 26, 2023

What?

Remove the flag that was hiding Block Theme Previews behind the experiment option, as we addressed issues in this comment #50030 (comment).

Related PRs: #50030, #50863

Why?

Since I assume there's no blocker to remove the flag. Let me know if we have something! 🙏

I refer to the following issues/feedback that we could work on them to improve the experience after this PR;

☂️

How?

  • Removed a setting field for gutenberg-theme-previews
  • Removed the condition for the experiment

We will need to stop adding the Preview button when https://core.trac.wordpress.org/ticket/58190 lands, as it's written in https://github.com/WordPress/gutenberg/blob/release/15.8/lib/compat/wordpress-6.3/theme-previews.php#L56

Testing Instructions

To see Block Theme Previews are working;

  • Go to /wp-admin/themes.php
  • Click the Live Preview button on a theme installed

or

  • Open the Site Editor and add the following to your URL: ?theme_preview=[themePath] where themePath is the relative path to the theme you want to preview (e.g. twentytwentythree).

To see that there's no Block Theme Previews option;

  • Go to /wp-admin/admin.php?page=gutenberg-experiments

Screenshots or screencast

Screen Shot 2023-05-26 at 10 40 29 Screen Shot 2023-05-26 at 10 39 44

@okmttdhr okmttdhr marked this pull request as ready for review May 26, 2023 01:44
@okmttdhr okmttdhr requested a review from spacedmonkey as a code owner May 26, 2023 01:44
Copy link
Contributor

@ajlende ajlende left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Nothing under the ☂️ seems like a deal breaker to me, but I'll hold off from clicking the merge button in case someone else has a different opinion.

@okmttdhr okmttdhr marked this pull request as draft May 31, 2023 01:17
@okmttdhr
Copy link
Contributor Author

okmttdhr commented May 31, 2023

Thanks for your review!
I marked this PR as a draft since the GET variable theme_preview is something widely used in the WordPress context, and some 3rd parties may have conflicting uses. (e.g. WordPress.com is using $_GET['theme_preview'] as well).

I'll create follow-up PRs to prevent that.

@okmttdhr
Copy link
Contributor Author

okmttdhr commented Jun 6, 2023

CC: @scruffian @ajlende @draganescu
I've made two changes before this PR merged.

@okmttdhr okmttdhr marked this pull request as ready for review June 6, 2023 02:08
@okmttdhr okmttdhr marked this pull request as draft June 8, 2023 01:14
@okmttdhr okmttdhr marked this pull request as ready for review June 8, 2023 06:39
@scruffian
Copy link
Contributor

I noticed that the button text for activating the theme has been modified and now doesn't make sense when you're previewing a theme:

Screenshot 2023-06-08 at 09 40 51

It would be good to fix this before we merge this PR.

Copy link
Contributor

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

Now #51361 is merged I think we can also bring this in.

@scruffian scruffian merged commit 444697d into WordPress:trunk Jun 9, 2023
@github-actions github-actions bot added this to the Gutenberg 16.1 milestone Jun 9, 2023
@okmttdhr okmttdhr deleted the update/remove-block-theme-previews-experiment branch June 12, 2023 02:12
sethrubenstein pushed a commit to pewresearch/gutenberg that referenced this pull request Jul 13, 2023
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