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

Ensure the Twenty Twenty-Two theme is AMP compatible #6649

Closed
pierlon opened this issue Oct 13, 2021 · 9 comments · May be fixed by #6870
Closed

Ensure the Twenty Twenty-Two theme is AMP compatible #6649

pierlon opened this issue Oct 13, 2021 · 9 comments · May be fixed by #6870
Assignees
Labels
Enhancement New feature or improvement of an existing one P1 Medium priority
Milestone

Comments

@pierlon
Copy link
Contributor

pierlon commented Oct 13, 2021

Feature Description

Development of the Twenty-Two core theme is now underway and is set to be released along with WP 5.9 in December. We should ensure it will be AMP compatible and also add it to the list of Reader themes.

Acceptance Criteria

  • Ensure sure the Twenty-Two theme is AMP compatible
  • Add the theme as a Reader theme option

Implementation Brief

No response

QA Testing Instructions

No response

Demo

No response

Changelog Entry

No response

@pierlon pierlon added the Enhancement New feature or improvement of an existing one label Oct 13, 2021
@westonruter
Copy link
Member

Since this theme depends on full-site editing, it doesn't seem like there will be anything in the theme itself which is not AMP-compatible, as it is comprised of blocks coming from core. Therefore it's likely the majority or the work here will be to ensure that FSE is AMP-compatible.

For example: #6115 and #6319.

@westonruter
Copy link
Member

So the good thing here is that if we can ensure AMP is FSE-compatible, then any such theme will just work.

@delawski
Copy link
Collaborator

QA Not Passed

Ensure sure the Twenty-Two theme is AMP compatible

I've browsed a site based on WordPress 5.9 with Twenty-Two theme activated and everything seemed to be working as expected, including items like site navigation (navigation block), adding comments, and search.

There were no AMP validation errors reported.

🚫 Add the theme as a Reader theme option

Twenty-Two is not among Reader themes:

AMP Onboarding Wizard ‹ amp-qa — WordPress

@westonruter
Copy link
Member

If the only fail is the theme not being listed among the Reader themes, then great. We can easily add it. We'll just need a mobile screenshot to add to https://github.com/ampproject/amp-wp/tree/develop/assets/images/reader-themes

@delawski
Copy link
Collaborator

delawski commented Jan 31, 2022

If the only fail is the theme not being listed among the Reader themes, then great. We can easily add it. We'll just need a mobile screenshot to add to https://github.com/ampproject/amp-wp/tree/develop/assets/images/reader-themes

Thanks for the hint. I've added a Twenty Twenty-Two thumbnail to the repo and added its slug to the list of supported Reader Themes in #6870.

One unexpected thing I noticed is that the "Edit site" menu item is rendered in WP admin bar when TT2 is set as a Reader Theme:

Screenshot 2022-01-31 at 15 56 40

After clicking on the link, I get an error like:

Screenshot 2022-01-31 at 16 02 02

I think that there might be more of such little things here and there WordPress 5.9 and TT2 introduced quite a bit of changes.

@westonruter
Copy link
Member

After clicking on the link, I get an error like:

Do you get the same issue when the AMP plugin is deactivated?

@delawski
Copy link
Collaborator

delawski commented Jan 31, 2022

Do you get the same issue when the AMP plugin is deactivated?

No, when I select TT2 as a regular theme in Standard or Transitional mode (not as a Reader Theme), the link leads to the new site editor. In the same way, if AMP is completely deactivated, the link works as expected.

So this is only the case if TT2 is set as a Reader Theme. I guess we should probably remove this link from the admin bar if in Reader Mode. What do you think?

@westonruter
Copy link
Member

westonruter commented Jan 31, 2022

So this is only the case if TT2 is set as a Reader Theme. I guess we should probably remove this link from the admin bar if in Reader Mode. What do you think?

Oh yes, you're right. Humm, in that case I think we'll need to wait on adding TT2 to the list of Reader themes as we'll need to make the Site Editor accessible with the Reader theme active, in the same way we load the Reader theme when loading the AMP Customizer. Otherwise the user wouldn't be able to edit the AMP pages. It may end up being that the Site Editor can't be forced to load the Reader theme, but we'll need to do more research in that area. So I'll move the milestone to 2.3 for now. Thanks for identifying this!

@westonruter westonruter modified the milestones: v2.2.1, v2.3 Jan 31, 2022
@westonruter
Copy link
Member

This is done.

@westonruter westonruter modified the milestones: v2.3, v2.2 Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or improvement of an existing one P1 Medium priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants