amp-wp
amp-wp copied to clipboard
Eliminate "AMP Settings" panel from Gallery block
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:
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:
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
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.
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
andamp-lightbox
attributes into thegallery
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.
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)?
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.
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.
QA Passed ✅
Cross-checked the issue, The AMP settings related to the Gallery block are moved to the advance settings.
However, Could we add some label for it similar to HTML ANCHOR/ADDITIONAL CSS CLASS(ES)?
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 Good point, yes. Please add a heading for those two toggles.