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

Eliminate "AMP Settings" panel from Gallery block #4989

Closed
westonruter opened this issue Jul 5, 2020 · 9 comments · Fixed by #7171
Closed

Eliminate "AMP Settings" panel from Gallery block #4989

westonruter opened this issue Jul 5, 2020 · 9 comments · Fixed by #7171
Labels
Changelogged Whether the issue/PR has been added to release notes. Editor Groomed Integration: WP Core P2 Low priority Punted WS:Core Work stream for Plugin core
Projects
Milestone

Comments

@westonruter
Copy link
Member

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 westonruter added this to the v1.7 milestone Jul 5, 2020
@westonruter westonruter added this to Backlog in Ongoing via automation Jul 5, 2020
@kmyram kmyram added the WS:Core Work stream for Plugin core label Aug 5, 2020
@rsmith4321
Copy link

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.

@westonruter
Copy link
Member Author

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 #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.

@huntlyc
Copy link

huntlyc commented Sep 9, 2020

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)?

@westonruter
Copy link
Member Author

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.

@kmyram kmyram added the Groomed label Sep 15, 2020
@kmyram kmyram moved this from Backlog to To Do in Ongoing Sep 15, 2020
@jwold
Copy link
Collaborator

jwold commented Sep 15, 2020

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.

@pavanpatil1
Copy link

QA Passed ✅

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

image

@pavanpatil1
Copy link

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

@thelovekesh thelovekesh moved this from Ready for QA to QA Passed in Ongoing Feb 6, 2023
@thelovekesh
Copy link
Collaborator

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.

@westonruter
Copy link
Member Author

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

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. Editor Groomed Integration: WP Core P2 Low priority Punted WS:Core Work stream for Plugin core
Projects
Ongoing
  
QA Passed
7 participants