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.

Sign up for GitHub

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

Jump to bottom

Add support for captions to the Gallery shortcode #3658

Closed
kienstra opened this issue Oct 29, 2019 · 8 comments · Fixed by #3659
Closed

Add support for captions to the Gallery shortcode #3658

kienstra opened this issue Oct 29, 2019 · 8 comments · Fixed by #3659
Assignees
@kienstra
Milestone
v1.5

Comments

@kienstra
Copy link
Contributor

kienstra commented Oct 29, 2019 •
edited
Loading

Feature description

The [gallery] shortcode does not display captions. It'd be good to add support for them, as the Classic editor uses this for galleries.

A follow-up to #2855

Acceptance criteria

  1. If a [gallery] shortcode has images with captions, the captions should display in the <amp-carousel>.
  2. Images without captions should still look good.

Implementation brief

QA testing instructions

  1. In /wp-admin, click 'Media'
  2. Click any image:

media-here-image

  1. Add a caption, and make note of the ID:

add-caption

  1. Repeat steps 2-3
  2. You should now have at least 2 IDs for images
  3. Create a new post in the block editor
  4. Add a 'Shortcode' block
  5. Add a Gallery shortcode, with IDs of the images from step 2. For example, if the IDs were 4572 and 3619, the shortcode would be:
[gallery ids="4572, 3619"]
  1. Click 'Preview,' and go to the AMP URL
  2. Expected: The captions should be visible in the gallery
  3. Repeat steps 1-9, with galleries that have no images with captions, very long captions, only a few images with captions, etc...
  4. Please re-test the Gallery block testing steps. PR Enable captions on Gallery shortcodes #3659 refactors that code, but there's no change intended to the Gallery block.

Demo

Changelog entry
@kienstra kienstra self-assigned this Oct 29, 2019
@kienstra kienstra mentioned this issue Oct 29, 2019
Merged
3 tasks
@kienstra kienstra assigned westonruter and unassigned kienstra Oct 31, 2019
@westonruter westonruter added this to the v1.5 milestone Nov 26, 2019
@csossi
Copy link

csossi commented Dec 7, 2019

Verified in QA:
image

@csossi
Copy link

csossi commented Dec 7, 2019

@kienstra , I retested as per step 12 and opted to "display as carousel" but AMP version of page isn't appearing as carousel:
image

@csossi csossi assigned kienstra and unassigned westonruter Dec 7, 2019
@kienstra
Copy link
Contributor Author

kienstra commented Dec 8, 2019

Hi @csossi,
Thanks a lot for testing this.

I'll make a note to look at this again, though it might be a few days.

@kienstra
Copy link
Contributor Author

Hi @csossi,
Thanks for testing the Gallery block.

Maybe this wasn't the case before, but on the testing site, validation looks to be failing, and it's not actually an AMP URL:

https://story-test-wordpress-amp.pantheonsite.io/?p=5653
amp-validation

On clicking 'Validate,' this error appears:
validation-eher

It might be hard to diagnose that error without the error log, but the Gallery block appears as a carousel in my local when 'Display as carousel' is checked:

gallery-block-elc

@kienstra
Copy link
Contributor Author

Also, sometimes there's a lightbox for the Gallery block, even with 'Display as carousel' and 'Add lightbox effect' unselected.

But it looks like this is from 'auto-lightboxing':

https://github.com/ampproject/amphtml/blob/e33dd10533805655835c196389e4b5ea91f080b1/spec/auto-lightbox.md

@kienstra
Copy link
Contributor Author

If it's alright, I'm going to merge this to 'Done.' Feel free to reply if there are more points to bring up.

@westonruter
Copy link
Member

It might be hard to diagnose that error without the error log,

You can see the error at the bottom of the page:

Fatal error: Uncaught Error: Class 'Amp\AmpWP\Component\DOMElementList' not found in /srv/bindings/842e702ecb134f309437791560d11313/code/wp-content/plugins/amp/includes/sanitizers/class-amp-gallery-block-sanitizer.php:109
Stack trace:
#0 /srv/bindings/842e702ecb134f309437791560d11313/code/wp-content/plugins/amp/includes/templates/class-amp-content-sanitizer.php(117): AMP_Gallery_Block_Sanitizer->sanitize()
#1 /srv/bindings/842e702ecb134f309437791560d11313/code/wp-content/plugins/amp/includes/class-amp-theme-support.php(2315): AMP_Content_Sanitizer::sanitize_document(Object(DOMDocument), Array, Array)
#2 /srv/bindings/842e702ecb134f309437791560d11313/code/wp-content/plugins/amp/includes/class-amp-theme-support.php(2003): AMP_Theme_Support::prepare_response('<!DOCTYPE html>...')
#3 [internal function]: AMP_Theme_Support::finish_output_buffering('<!DOCTYPE html>...', 9)
#4 /srv/bindings/842e702ecb134f309437791560d11313/code/wp-includes/functions.php(4469): ob_end_flush()
#5 /srv/bindings/842e702ecb134f309437791560d11313/code in /srv/bindings/842e702ecb134f309437791560d11313/code/wp-content/plugins/amp/includes/sanitizers/class-amp-gallery-block-sanitizer.php on line 109

@westonruter
Copy link
Member

Fixed by #3921.

@westonruter westonruter mentioned this issue Dec 24, 2019
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kienstra kienstra

Labels
None yet
Projects
None yet
Milestone
v1.5
Development

Successfully merging a pull request may close this issue.

Enable captions on Gallery shortcodes ampproject/amp-wp
3 participants
@westonruter @kienstra @csossi