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

Gallery shortcodes unexpectedly show as carousels by default in Transitional/Standard mode #4774

Closed
westonruter opened this issue May 26, 2020 · 2 comments · Fixed by #4775
Closed
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. Embeds Groomed Integration: WP Core Needs sizing P0 High priority WS:Core Work stream for Plugin core
Milestone

Comments

@westonruter
Copy link
Member

Bug Description

When a site has galleries in Classic blocks, these are always getting displayed as carousels unless the author goes into the code editor and adds amp-carousel=false to the [gallery] shortcodes. This was done back at a time before the block editor existed and at a time in which the gallery block styles couldn't be rendered in Reader mode safely. Both of these are no longer the case, however. As of #4204 via #4299, core block styles are now enqueued in the classic post templates.

For sites in Standard/Transitional mode, the Shortcode block was augmented to inject amp-carousel=false into the block behind the scenes, while also exposing a new carousel toggle in the block settings. (This was not being done for shortcodes in Classic blocks however.) In hindsight, this seems irrelevant because when a Classic block is converted to blocks, the gallery shortcodes are not converted into Shortcode blocks but rather Gallery blocks. Therefore, all that logic to extend the Shortcode block in the block editor should just be removed.

👉 The gallery shortcode should not display as a carousel in Standard/Transitional mode unless explicitly requested via the amp-carousel=true shortcode attribute.

Also, it may make sense for the Gallery block to keep the carousel enabled by default when in Reader mode. Nevertheless, when the [gallery] shortcode does have amp-carousel=false, we should make sure that the required gallery styles are enqueued in classic Reader mode (or else create such basic styles).

Reported in support forum topic.

Expected Behaviour

Steps to reproduce

  1. Create a Classic block.
  2. Add a gallery.
  3. View the post in non-AMP, and see it is displayed as a grid.
  4. View the post in AMP Reader mode: see a carousel.
  5. View the post in AMP Transitional mode: also see a carousel (but this is not expected).

Screenshots

Editor

image

Non-AMP ✅

image

AMP Reader Mode ✅

image

AMP Transitional mode ❌

image


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
Copy link
Member Author

Here's a workaround plugin to prevent gallery shortcodes from rendering as carousels by default: https://gist.github.com/westonruter/98e2e6559389840111b430ab4db2075d

@kmyram kmyram added this to the v1.6 milestone May 27, 2020
@westonruter westonruter added the P0 High priority label Jun 10, 2020
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Aug 1, 2020
@kmyram kmyram added the WS:Core Work stream for Plugin core label Aug 5, 2020
@kmyram kmyram assigned johnwatkins0 and unassigned pierlon Aug 24, 2020
@johnwatkins0
Copy link
Contributor

Confirming this is fixed and passes QA.

  1. Created a gallery in a Classic block
  2. It showed as a grid in all non-AMP or Standard versions of the post
  3. It showed as a carousel in the legacy theme in Reader mode

@johnwatkins0 johnwatkins0 removed their assignment Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. Embeds Groomed Integration: WP Core Needs sizing P0 High priority WS:Core Work stream for Plugin core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants