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

Update Gutenberg packages after v12.5.0 release #6881

Merged
merged 15 commits into from
Feb 3, 2022

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Feb 3, 2022

  • Verify Preview button shows up in older versions of WordPress without Gutenberg as well.

pierlon and others added 11 commits January 28, 2022 00:03
…tenberg-v12.4.1-packages

* 'develop' of github.com:ampproject/amp-wp:
  Bump node-fetch from 2.6.1 to 2.6.7 in /.github/actions/draft-release
  Bump lint-staged from 11.2.6 to 12.3.3
  Bump mustache/mustache from 2.13.0 to 2.14.1
  Bump cweagans/composer-patches from 1.7.1 to 1.7.2
  Bump follow-redirects from 1.14.5 to 1.14.7
  Bump webpackbar from 5.0.0-3 to 5.0.2
@westonruter westonruter added the dependencies Pull requests that update a dependency file label Feb 3, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2022

Plugin builds for 15c713c are ready 🛎️!

…g-v12.5.0-packages

* update/gutenberg-v12.4.1-packages:
  Fix JS import order
  Update Gutenberg package dependencies
  Update Gutenberg package dependencies
  Update snapshots
  Attempt to fix error to resolve a module for a custom reporter
  Fix eslint errors
  Update Gutenberg package dependencies
@westonruter
Copy link
Member Author

#6858 (comment):

The E2E test is failing due to the .editor-post-preview element being after the .amp-wrapper-post-preview element, whereas previously it was before (and it is being inserted as such). I saw this locally as well for a bit, but now I don't any longer 😕

#6858 (comment):

In testing this with Gutenberg 12.5, it seems the problem is that WrappedAmpPreviewButton is constructed before the .editor-post-preview element has been added to the DOM, so this.postPreviewButton is null here:

this.postPreviewButton = document.querySelector( `.${ POST_PREVIEW_CLASS }` );

https://github.com/ampproject/amp-wp/runs/5046692459?check_suite_focus=true#step:12:1

image

@westonruter westonruter requested a review from delawski February 3, 2022 04:00
@delawski
Copy link
Collaborator

delawski commented Feb 3, 2022

In testing this with Gutenberg 12.5, it seems the problem is that WrappedAmpPreviewButton is constructed before the .editor-post-preview element has been added to the DOM

That's interesting. Mu gut feeling was that it has something to do with the fact that we're using class components for the preview button. It seems like they are now being constructed before the editor is set up.

So I went ahead and rewritten the class component into a functional component. It seems that it helped. The preview button is now back in its original position.

However, there's a slight regression when Gutenberg is turned off. The preview button is added a bit too late causing a horizontal layout shift:

preview-button-flicker.mov

I'm going to dive deeper and see if this can be easily fixed.

The post Publish/Update button is rendered early on (contrary to post Preview button) and so it's safer to use it to imperatively add an AMP Preview button to the block editor markup.

Also, the `viewable` property of a post type get defined late, similarly to when the post Preview button gets added. That's why we assume a post type is viewable and show the AMP Preview button initially. Only if we learn that a post type is not viewable, we hide the AMP Preview button.
@delawski
Copy link
Collaborator

delawski commented Feb 3, 2022

@westonruter I think I found a way to prevent the flicker, at least for the majority of use cases. Check out a message in 9f3e1c8.

@delawski
Copy link
Collaborator

delawski commented Feb 3, 2022

Oh, there's a side-effect of the recent changes that also makes the E2E tests fail. The AMP preview button is now in a different location on mobile:

Screenshot 2022-02-03 at 19 00 18

I'll quickly check out if there's a way to put it back in place.

@westonruter
Copy link
Member Author

In terms of the shift, this is looking good with Gutenberg and without Gutenberg, and when the post post type is enabled and disabled. The following videos all show the same good behavior:

non-gutenberg.mov
gutenberg.mov
gutenberg-with-post-type-disabled.mov
no-gutenberg-with-post-type-disabled.mov

In the case for when the post type is disabled, the scripts aren't even enqueued:

// Only enqueue scripts on the block editor for AMP-enabled posts.
$editor_support = Services::get( 'editor.editor_support' );
if ( ! $editor_support->is_current_screen_block_editor_for_amp_enabled_post_type() ) {
return;
}

@westonruter
Copy link
Member Author

westonruter commented Feb 3, 2022

And the only post types that are eligible for being AMP-enabled are those which have 'public' => true:

public static function get_eligible_post_types() {
$post_types = array_values(
get_post_types(
[
'public' => true,
],
'names'
)
);
/**
* Filters the list of post types which may be supported for AMP.
*
* By default the list includes those which are public.
*
* @since 2.0
*
* @param string[] $post_types Post types.
*/
return array_values( (array) apply_filters( 'amp_supportable_post_types', $post_types ) );
}

It's true that a post type could have public set to true but have publicly_queryable set to false, I think, and in that case it may be possible that we could accidentally show or hide the preview button. It would be marginally better to here instead do the following since there is now a is_post_type_viewable filter in WP 5.9:

diff --git a/includes/class-amp-post-type-support.php b/includes/class-amp-post-type-support.php
index 3d4560641..17eb6519b 100644
--- a/includes/class-amp-post-type-support.php
+++ b/includes/class-amp-post-type-support.php
@@ -42,14 +42,9 @@ public static function get_builtin_supported_post_types() {
 	 * @return string[] Post types eligible for AMP.
 	 */
 	public static function get_eligible_post_types() {
-		$post_types = array_values(
-			get_post_types(
-				[
-					'public' => true,
-				],
-				'names'
-			)
-		);
+		$post_types = array_values( get_post_types( [], 'names' ) );
+
+		$post_types = array_filter( $post_types, 'is_post_type_viewable' );
 
 		/**
 		 * Filters the list of post types which may be supported for AMP.

But that can be done later if needed. I'll open an issue.

@delawski
Copy link
Collaborator

delawski commented Feb 3, 2022

I've pushed another update (15c713c). This time, we're adding the AMP Preview button in two rounds.

As long as the Preview button (the one that we've used so far) is not in the DOM, we'll adding our AMP Preview button before Publish/Update button.

Right after the Preview button is added to the editor (we use isViewable as an imperfect indicator of that), the AMP Preview button gets re-rendered in the correct location.

I haven't ran E2E tests locally. It may turn out that E2E tests are "too quick" and will find our AMP Preview button before it gets mounted in the correct location. In such a case, we can increase the wait time for the particular assessment or make the AMP Preview button selector more liberal (e.g. just check if the button is a child of .edit-post-header__settings element, and not try confirming its exact location).

@westonruter
Copy link
Member Author

make the AMP Preview button selector more liberal (e.g. just check if the button is a child of .edit-post-header__settings element, and not try confirming its exact location).

I was also considering this myself.

@westonruter
Copy link
Member Author

It passed!

@westonruter westonruter merged commit 203d057 into develop Feb 3, 2022
@westonruter westonruter deleted the update/gutenberg-v12.5.0-packages branch February 3, 2022 18:53
@westonruter
Copy link
Member Author

@delawski I've just noticed an issue: the preview button is not showing up in WP 5.6, 5.7, and 5.8 until the user makes a change and then saves a draft.

wp-5.6-amp-preview-only-appears-after-saving.mov

It seems the container is empty:

image

And also, when I load a post that is published I see the button show up for a moment and then disappear until I make a change and save the post:

momentary-dispay-of-preview-button-when-published.mov

It doesn't happen in either 5.9 or in 5.7 or 5.8 with the latest Gutenberg is active.

So I don't consider this a blocker for 2.2.1, but it should be fixed in 2.2.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants