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

Add AMP enabled/disabled toggle to Gutenberg editor #1230

Closed
westonruter opened this Issue Jun 27, 2018 · 11 comments

Comments

5 participants
@westonruter
Copy link
Member

commented Jun 27, 2018

The classic post editor has a toggle to enable/disable AMP for a given post:

image

This needs to be added to Gutenberg as well.

@westonruter westonruter added this to the v1.0 milestone Jun 27, 2018

@postphotos postphotos added the release label Jul 9, 2018

@postphotos postphotos added this to Definition in v1.0 Jul 9, 2018

@westonruter westonruter moved this from Definition to To do in v1.0 Jul 9, 2018

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2018

@jwold This needs some Gutenberg design insights.

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2018

The AMP-specific blocks should not be made available on the Inserter when AMP is disabled for a given post, even in native mode.

@jwold

This comment has been minimized.

Copy link

commented Jul 10, 2018

@westonruter what would happen in this scenario:

  1. Enable AMP in a Gutenberg post
  2. Add an AMP-specific block
  3. Disable Gutenberg
  4. What happens to that block that's already on the page? Should we display a notice around it that it's incompatible, should we gray it out?
@jwold

This comment has been minimized.

Copy link

commented Jul 10, 2018

screen shot on 2018-07-10 at 08_04_29

For the interface in Gutenberg I'm thinking something as simple as this ^

Any concerns?

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2018

@jwold that mock looks good to me. I'm assuming Gutenberg has is extensible to allow new toggles to be added in that location.

What happens to that block that's already on the page? Should we display a notice around it that it's incompatible, should we gray it out?

Good question. In the case where you've added an AMP-specific block to content but then you disable AMP, the next time you access the post then the block would be unrecognized and it would show the same notice as if a block from a plugin were in content but then the plugin was deactivated:

image

This seems acceptable to me.

@jwold

This comment has been minimized.

Copy link

commented Jul 10, 2018

In the case where you've added an AMP-specific block to content but then you disable AMP, the next time you access the post then the block would be unrecognized and it would show the same notice as if a block from a plugin were in content but then the plugin was deactivated:

I'm good with this approach, my only question is if we should change the wording at all to be specific, such as,

"This block requires enabling AMP in the document settings"
[ Re-enable AMP button ] [ Edit as HTML button ]

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2018

That screenshot is just the default view as presented in Gutenberg for a block that is no longer recognized. I'm not sure how easy it is to customize it for specific blocks. That would be nice for the AMP case here.

@kienstra

This comment has been minimized.

Copy link
Collaborator

commented Jul 16, 2018

Here's a document on how to add a row in the Gutenberg "Status & Visibility" panel:

https://wordpress.org/gutenberg/handbook/packages/packages-edit-post/#pluginpoststatusinfo

@kienstra kienstra self-assigned this Jul 16, 2018

@kienstra kienstra moved this from To do to In progress in v1.0 Jul 16, 2018

@kienstra kienstra referenced this issue Jul 21, 2018

Merged

Add Gutenberg 'Enable AMP' toggle #1275

2 of 2 tasks complete

@westonruter westonruter moved this from In progress to Ready for QA in v1.0 Aug 1, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

commented Aug 3, 2018

Request For Testing

Hi @csossi,
Could you please test this?

  1. Create a new post, using Gutenberg
  2. Expected: the "Enable AMP" toggle is on

post-block-editor

  1. Add a title to the new post, then click "Save Draft" and "Preview"
  2. Add &amp to the URL, and go to that address.
  3. Expected: it's a valid AMP page, per the Chrome AMP Extension:

valid-amp-page

  1. Go back to the editor, uncheck the "Enable AMP" toggle, and click "Save Draft"
  2. Repeat step 4.
  3. Expected: The page redirects to the URL without &amp appended, as the AMP page isn't available
  4. Go to the AMP settings page, ensure "Paired" is selected, and uncheck "Pages":

amp-general-settings

  1. Create a new page using Gutenberg:
  2. Expected: There's no "Enable AMP" toggle, only a notice:

amp-notice

12. Check the "Pages" box again from step 9, and click "Save Changes"

@kienstra kienstra assigned csossi and unassigned kienstra Aug 3, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

commented Sep 5, 2018

Request For Regression Testing

Hi @csossi,
Could you please retest this to ensure there wasn't a regression, using the test steps above?

Here's the staging site:
https://paired-ampconfdemo.pantheonsite.io/

Thanks, Claudio!

@csossi

This comment has been minimized.

Copy link
Collaborator

commented Sep 30, 2018

Verified in QA

@csossi csossi moved this from Ready for QA to Ready for Merging in v1.0 Sep 30, 2018

@csossi csossi removed their assignment Sep 30, 2018

@kienstra kienstra moved this from Ready for Merging to In Production in v1.0 Dec 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.