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

Merge AMP Customizer with main Customizer #796

Closed
westonruter opened this issue Nov 6, 2017 · 14 comments

Comments

Projects
7 participants
@westonruter
Copy link
Member

commented Nov 6, 2017

[Edited] Refer to the latest [acceptance criteria] #796 (comment)


Currently if you want to Customizer your AMP template you have to go to Appearance > AMP in the admin, and this will open the Customizer in a blank slate with only the AMP panel registered. The latest post is loaded in to the preview, and navigation of the site is disabled. There are a couple problems with this:

  1. It is difficult to discover the AMP Customizer if you don't know specifically to look for that Appearance > AMP option.
  2. If you add AMP support for other post types, then you can't preview how they look in the Customizer since they cannot be navigated to.

So I propose that we merge the AMP panel into the regular Customizer. We can allow the user to navigate around the entirety of their site in the Customizer preview, but then depending on whether or not the AMP panel is expanded then automatically toggle the preview URL to include the AMP version if it is available. If AMP is not available for a given URL then the Customizer can show a notification in the AMP panel or any of its sub-sections that AMP is not available, and that they should navigate to a URL that is AMP compatible. The notification could have a link to take them to the most recent post in the preview.

There can still be an Appearance > AMP link in the admin menu, but it can just deep-link to the AMP panel and supply the url of the most recent post as the preview to start with.

I consider this to be a dependency for adding page support (#176), because if you can't navigate to pages (or other custom post types) in the Customizer, then you can't customize their appearance.

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Nov 6, 2017

In paired mode (the only mode presently), there can be an AMP toggle that appears with the device preview buttons, where you can decide whether or not you want to preview the AMP version or not, if it is available. When in canonical mode (#668) then this button would not be needed because every URL would already be in AMP mode. Alternatively, we could always automatically show the AMP version when the Mobile device is being previewed, though since AMP is not specific to mobile anymore, this is something that should be previewable regardless of the device type.

@westonruter westonruter added this to the v0.6 milestone Nov 7, 2017

@westonruter westonruter added this to To Do in v0.6 Nov 7, 2017

@westonruter westonruter self-assigned this Nov 7, 2017

@MackenzieHartung

This comment has been minimized.

Copy link
Collaborator

commented Nov 27, 2017

Acceptance criteria:

  1. The existing AMP panel will be merged into the main Customizer.
  2. Once in the Customizer, with the AMP panel expanded, the user will be able to navigate throughout the site, previewing each page with the AMP URL.
  3. In the event a user navigates to a page that is not AMP enabled a notification will display within the AMP panel to inform the user that they should navigate to a URL that is AMP compatible.
  4. There will be an Original/AMP toggle, located by the device size buttons within the Customizer, allowing a user to toggle to view the AMP preview, even if the AMP panel within the Customizer is closed.
  5. When the AMP Panel is open this new toggle will automatically default to AMP, and be disabled.
  6. When a user selects to view the mobile version of a page that is AMP supported, the new toggle will automatically default to AMP. The user will be able to change it to Original if they wish to do so. If the user switches back to desktop, then AMP will then be auto-disabled.
  7. A user will be able to select a specific device along with the new toggle to view Original or AMP preview in the various screen sizes.
  8. The toggle/device size preferences will carry through as the user navigates throughout the site.
  9. In the event a user navigates to a page that is not AMP enabled, and the AMP Panel is not open, a notification will display in the notification area for the panel/section. Additionally, there will be a treatment applied to the AMP preview toggle (as it will be set to off, and unable to be turned on). The treatment (which could be a glowing animation) will draw the user’s attention to the button so they are aware the page is not AMPifiable.
  10. The notification that displays when the page in view is not AMP compatible will include a link to the latest Post, enabling the user to see an AMP previewable page.
  11. The existing “Appearance > AMP” menu item in the admin will still exist. However, selecting this link will launch the Customizer, for the page the user selected the option from, with the AMP panel exposed.
  12. If a user accesses the Customizer, with the AMP panel expanded from a page/post that is not AMPlifiable, we will redirect the preview to the latest post the notification will be displayed with a link to preview a URL that is compatible with AMP.

@kaitnyl kaitnyl moved this from To Do to In Progress in v0.6 Nov 28, 2017

@MaximSiebert

This comment has been minimized.

Copy link

commented Nov 29, 2017

Explored what the 'Preview AMP' toggle could look like, can be seen in the figma file here.

Including a screenshot for reference.

screen shot 2017-11-29 at 4 19 58 pm

The top section shows the preview AMP toggle active, the second shows it disabled. The third one shows what would happen if a user navigates from an article page to the homepage where AMP is not supported – an alert would appear for 5 - 8 seconds and then fade out. This alert would reappear if the user hovers the toggle while on a page that isn't AMP compatible.

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2017

I like how the toggle style is similar to what is in Gutenberg:

image

I also like how the tooltip has the notification as part of the tooltip.

We should, however, make sure that all of this fits on one line. I think the Preview toggle should be positioned directly tho the left of the Desktop icon, with a faint separator between them to indicate it's a different kind of device preview toggle. I'm hoping we can hide the “Preview AMP” label, and make it part of a tooltip? The “Hide Controls” label could be hidden if we must.

@MaximSiebert

This comment has been minimized.

Copy link

commented Nov 30, 2017

Makes sense to me Weston! Here's and updated screenshot, I bumped the icon and font size of 'hide controls' to fit the amp toggle.

screen shot 2017-11-30 at 12 00 58 pm

@kaitnyl kaitnyl referenced this issue Dec 6, 2017

Merged

#796 Move AMP Customizer into main Customizer #819

2 of 3 tasks complete

@kaitnyl kaitnyl moved this from In Progress to Ready for Review in v0.6 Dec 6, 2017

@ThierryA ThierryA moved this from Ready for Review to In Progress in v0.6 Dec 6, 2017

@kaitnyl kaitnyl moved this from In Progress to Ready for Review in v0.6 Dec 8, 2017

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2017

I made a couple tweaks to the ACs 6 & 12. I added ins/del markup to show what I changed.

@csossi

This comment has been minimized.

Copy link
Collaborator

commented Dec 12, 2017

@ThierryA @westonruter Comments re: use case
Even with the notifications and warnings, user can still go ahead and make changes in the AMP panel and click "PUBLISHED". The changes won't appear live since AMP is disabled, but if AMP is subsequently enabled for some reason, then these now previous changes will take effect. Should this mean that the AMP panel in customizer be set to not open at all for an AMP-disabled page? ("Why bother opening and showing change options if it's not going to take effect anyway?") Thoughts, guys?
image

@csossi csossi moved this from QA to To Do in v0.6 Dec 12, 2017

@jwold

This comment has been minimized.

Copy link

commented Dec 12, 2017

If I go to Customize my site and land on a Non-AMPed page, the AMP toggle button displays correctly (it's in the off state with a notice about navigating to an AMP compatible page). If I click on the notice to go to an AMP compatible page, and then click on the logo to go back to the homepage the toggle shows up in a ON state but is disabled. Shouldn't the toggle now be set to an OFF state?

Video example: https://v.usetapes.com/FtlcBJY15l

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2017

Even with the notifications and warnings, user can still go ahead and make changes in the AMP panel and click "PUBLISHED". The changes won't appear live since AMP is disabled, but if AMP is subsequently enabled for some reason, then these now previous changes will take effect. Should this mean that the AMP panel in customizer be set to not open at all for an AMP-disabled page? ("Why bother opening and showing change options if it's not going to take effect anyway?") Thoughts, guys?

@jwold This is the infamous question behind https://core.trac.wordpress.org/ticket/40432

In other words, should the AMP panel be contextual based on whether AMP is is shown in the preview? If that is the case, then it would be harder to discover the AMP panel.

@csossi

This comment has been minimized.

Copy link
Collaborator

commented Dec 12, 2017

possibly solvable by having the notification saying that "AMP is disabled for page" include something like "note: any published revisions will not appear until AMP enabled" or something like that?

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2017

@jwold

If I go to Customize my site and land on a Non-AMPed page, the AMP toggle button displays correctly (it's in the off state with a notice about navigating to an AMP compatible page). If I click on the notice to go to an AMP compatible page, and then click on the logo to go back to the homepage the toggle shows up in a ON state but is disabled. Shouldn't the toggle now be set to an OFF state?

No, because AMP is still enabled but it is unavailable. So as soon as you land on a page or post again, then you'll get the AMP version not the non-AMPed version.

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2017

@csossi

possibly solvable by having the notification saying that "AMP is disabled for page" include something like "note: any published revisions will not appear until AMP enabled" or something like that?

This is a general UX problem for the Customizer: modifying settings which aren't displayed in the preview. For widgets the approach has been to hide the Customizer section entirely, but this introduces problems with discovery.

An alternative approach to hiding can be seen in the Header Media section in Twenty Seventeen. Only the video control is shown when you are previewing the homepage. If you are not previewing the homepage then the video control is hidden and a persistent notification is shown in it's place. Would such a persistent notification be better than the momentary tooltip?

@ThierryA

This comment has been minimized.

Copy link
Collaborator

commented Dec 12, 2017

How about replacing the AMP settings with a admin notice (within the panel) if the user is on a Non-AMP page or post?

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2017

Yes, re-using https://core.trac.wordpress.org/ticket/38794

For example:

image

@ThierryA ThierryA moved this from To Do to Ready for Merging in v0.6 Dec 13, 2017

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.