ampproject / amp-wp

Enable AMP on your WordPress site, the WordPress way.
https://wordpress.org/plugins/amp/
GNU General Public License v2.0
1.79k stars 384 forks source link

Empty AMP Settings panel for video block #6508

Closed swissspidy closed 2 years ago

swissspidy commented 3 years ago

Bug Description

I just inserted a video block in Gutenberg and selected a video and poster as usual.

There I noticed an empty "AMP Settings" panel in the block sidebar for this block.

Expected Behaviour

If there are no AMP settings, just don't render the panel.

Steps to reproduce

  1. Insert video block
  2. Notice empty AMP Settings panel in the block sidebar

Screenshots

Screenshot 2021-08-03 at 19 42 34

Additional context


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

westonruter commented 3 years ago

This looks to be something that was missed in https://github.com/ampproject/amp-wp/issues/4555 and/or https://github.com/ampproject/amp-wp/issues/4554 via https://github.com/ampproject/amp-wp/pull/5574 and https://github.com/ampproject/amp-wp/pull/5573. Namely, we've deprecated the AMP Layout setting, but we still show it if the user had previously set it while also showing a deprecation notice. What's missing, however, is to hide the panel entirely when neither of the toggles are present.

We left the toggles in place to give a chance for authors to give feedback if they were using them, but the only feedback we've received is not wanting to see a deprecation warning 😄

So for 2.2 let's go ahead and just remove the settings them entirely.

For 2.1.4 we could look at removing the panel if there aren't any items in it, but it may not be worth the effort to do both.

delawski commented 2 years ago

So for 2.2 let's go ahead and just remove the settings them entirely.

@westonruter Are we clear to remove everything related to ampLayout and ampNoLoading from the block editor sidebar for 2.3?

delawski commented 2 years ago

There's two more things I noticed. Even though they are not directly related to ampLayout or ampNoLoading described in this ticker, they are relevant to the AMP Settings panel contents.

  1. Should the "Add lightbox effect" toggle be visible for images inside a core gallery block or should this be controlled only at the gallery block level? Right now both the gallery and the images inside the gallery block display this toggle:
Gallery Image inside a gallery
Screenshot 2022-01-12 at 19 45 36 Screenshot 2022-01-12 at 19 45 52
  1. The AMP carousel on the frontend doesn't seem to be working (compared to the examples on amp.dev). The images are displayed next to each other and there are no controls to change slides. I haven't worked with the AMP carousel and AMP lightbox before too much so this note may be irrelevant.
AMP Plugin amp.dev
Screenshot 2022-01-12 at 19 49 45 Screenshot 2022-01-12 at 19 50 11
westonruter commented 2 years ago

@westonruter Are we clear to remove everything related to ampLayout and ampNoLoading from the block editor sidebar for 2.3?

Yes, let's. Nobody has raised any topics about wanting these settings.

1. Should the "Add lightbox effect" toggle be visible for images inside a core gallery block or should this be controlled only at the gallery block level? Right now both the gallery and the images inside the gallery block display this toggle:

No, they should not. The toggle should only be present for Image blocks that are not nested inside Gallery blocks.

3. The AMP carousel on the frontend doesn't seem to be working (compared to the examples on amp.dev). The images are displayed next to each other and there are no controls to change slides. I haven't worked with the AMP carousel and AMP lightbox before too much so this note may be irrelevant.

I think this may be a regression introduced by the change to how the Gallery block is constructed. I noticed something similar in https://github.com/ampproject/amp-wp/issues/6734#issuecomment-984228274.

dhaval-parekh commented 2 years ago

QA Passed

image