amp-wp icon indicating copy to clipboard operation
amp-wp copied to clipboard

Eliminate "AMP Settings" panel from Gallery block

Open westonruter opened this issue 4 years ago • 5 comments

Feature description

In the theme of preventing the user from having to know anything about AMP (#4554, #4555, #4556, #4557) to author content, we should also consider eliminating the dedicated “AMP Settings” panel from the Gallery block:

image

At one level this could just be a matter of removing “AMP” from the panel title. It could also mean moving the toggles under “Advanced”.

We also need to take into account how other plugins extend the Gallery block. Jetpack for example has this setting:

image

When that is enabled (as of Jetpack 8.7) the effect is as if the two toggles in AMP settings have been turned on. Users will be confused as to what purpose they have when this Jetpack feature is enabled.

Perhaps the AMP-provided settings panel should be made easy for another plugin to turn off (e.g. Jetpack).

We should also consider whether this functionality belongs in the AMP plugin in the first place, and if it should be removed/deprecated in favor of ecosystem plugins (like Jetpack).

Jetpack's implementation is key because it provides a non-AMP version on non-AMP pages. The AMP plugin doesn't currently provide that, which is yet another source of confusion for users. If a site is in a paired AMP setup (Reader/Transitional mode), why should there be AMP-specific settings present in the first place if functionality won't be working in the canonical (non-AMP) version of the page?

We should limit extending blocks with AMP functionality when that functionality can be used on any version of the page. This will be greatly facilitated by Bento AMP. However, in the meantime, we should consider whether this functionality should be in the AMP plugin or relegated to the ecosystem.


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 avatar Jul 05 '20 18:07 westonruter

I was confused why these settings were missing after the last update, I know most people have probably switched to a gallery block but I have reasons to use just the shortcode. So should we just add the settings into the shortcode now like "amp-lightbox=true amp-carousel=false"? I really do wish features weren't removed like this, I found them very useful and I wonder if anyone was actually confused by them.

rsmith4321 avatar Aug 29 '20 20:08 rsmith4321

You're using the Shortcode block with a [gallery]?

If so, then yes, if you want to continue using the [gallery] shortcode to display Galleries, you can manually supply the amp-lightbox and amp-carousel shortcode attributes, in the same way as you do now for ids. The reason why we removed these from the Shortcode block's settings when a [gallery] is supplied is it caused problems and users should be using the Gallery block instead anyway.

The PR that removed the toggles from the Shortcode block is https://github.com/ampproject/amp-wp/pull/4775. See this changelog entry:

  • Eliminate logic which attempts to inject amp-carousel and amp-lightbox attributes into the gallery shortcode block. Remove the toggles from the block settings. Adding galleries via the shortcode block should not need special block settings toggles, as these attributes should be added directly by the author. Galleries normally will only be added via the Gallery block or the Classic block.

westonruter avatar Aug 29 '20 20:08 westonruter

I would favour having a settings page like jetpack where you could toggle the carousel/lightbox options site-wide with it turned off by default.

Reason being, it took me longer than I care to admit to find out why a gallery was displaying as a carousel and how to stop it from doing so... 🤣

I do think this functionality belongs in the amp plugin as not everyone uses jetpack or, even if they do, they might already have rolled their own non-amp gallery/lightbox solution and turned those toggles off on jetpack (or any other media plugin that seems to provide their own gallery output these days)?

huntlyc avatar Sep 09 '20 16:09 huntlyc

Yeah, I could see the default values for whether the galleries are displayed as lightbox or carousel could be in an Advanced section on the AMP settings screen.

westonruter avatar Sep 09 '20 17:09 westonruter

Notes from backlog:

Related to AMP compatibility work for Jetpack. If you enable the Jetpack module or light boxes, it will enable Lightbox effect for all galleries. This doesn’t do anything, doesn’t make sense. Whether on or off you still get light boxes. We need a better way of representing this.

We could have global setting to enable all or off by default. Jetpack should hook into that so it’s consistent. Should also not say AMP. Should integrate more seamlessly into the block sidebar.

jwold avatar Sep 15 '20 16:09 jwold

QA Passed ✅

Cross-checked the issue, The AMP settings related to the Gallery block are moved to the advance settings.

image

pavanpatil1 avatar Feb 06 '23 08:02 pavanpatil1

However, Could we add some label for it similar to HTML ANCHOR/ADDITIONAL CSS CLASS(ES)?

pavanpatil1 avatar Feb 06 '23 08:02 pavanpatil1

However, Could we add some label for it similar to HTML ANCHOR/ADDITIONAL CSS CLASS(ES)?

@westonruter I think we can add a label AMP Settings as it will help understand where these settings are coming from.

JFI: Moved this to QA passed as functionality is working as expected.

thelovekesh avatar Feb 06 '23 08:02 thelovekesh

@thelovekesh Good point, yes. Please add a heading for those two toggles.

westonruter avatar Feb 06 '23 19:02 westonruter