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 image captions to the gallery carousel #2855

Closed
westonruter opened this issue Jul 19, 2019 · 17 comments · Fixed by #3285
Closed

Add image captions to the gallery carousel #2855

westonruter opened this issue Jul 19, 2019 · 17 comments · Fixed by #3285
Labels
Enhancement New feature or improvement of an existing one
Milestone
v1.4

Comments

@westonruter
Copy link
Member

Originally reported by @rsmith4321 on https://wordpress.org/support/topic/amp-carousel-with-captions/:

Is it possible to add captions the images in the amp gallery carousel? I know it is possible here https://amp.dev/documentation/examples/multimedia-animations/image_galleries_with_amp-carousel/ but it looks like it requires adding a caption div to each slide. Maybe this could be a future feature, thanks.

Should the captions be shown in the carousel slides by default or should there be a toggle to enable them?
@westonruter westonruter added the Enhancement New feature or improvement of an existing one label Jul 19, 2019
@rsmith4321
Copy link

An option would be nice, but really if the image has a caption I think it should be shown by default. That is the way it works with standard galleries.

@waviaei
Copy link

waviaei commented Jul 30, 2019

I agree with @rsmith4321 here. I am currently running into this issue, where we got a request to display caption texts when displaying carousel gallery in AMP mode, because that is how displayed in non-AMP mode. I now realize that both Block and Classic editor doesn't display caption texts.

@archon810
Copy link

Also agreed with how this should work.

@austintude
Copy link

I've just been trying to modify the caption styling of images in an amp-lightbox-gallery to no avail.
While I CAN style them in Chrome's Inspect Element with #amp-lightbox-gallery {font-size: 2rem; top:-4rem; position:relative;} when added to the <style amp-custom> these same styles appear to get shaken out by the AMP-plugin... Ideally we developers should have some controls allowed for the styling of the amp-lightbox-gallery and all(?) of its elements.

@westonruter
Copy link
Member Author

What style rules are you adding to the page?

I don't see any styling guidance on the amp-lightbox-gallery docs page. This may be something you should report with the AMP framework itself.

@austintude
Copy link

austintude commented Sep 10, 2019 via email

@westonruter
Copy link
Member Author

But what is the CSS selector you are using? I don't know what build process you're using for your CSS, but if #amp-lightbox-gallery {font-size: 2rem; top:-4rem; position:relative;} is in your CSS, then it should not get tree-shaken if you have an element with the ID of amp-lightbox-gallery on the page.

@austintude
Copy link

austintude commented Sep 10, 2019 via email

@westonruter westonruter mentioned this issue Sep 11, 2019
Draft
@westonruter
Copy link
Member Author

TIL. Yes, I see. Yes, the amp-lightbox-gallery element is created dynamically. It has the ID of amp-lightbox-gallery. This is why it is tree-shaken. Please try the PR #3226 which fixes this issue, but it's not clear yet whether the component should be styled directly.

@austintude
Copy link

austintude commented Sep 11, 2019 via email

@westonruter
Copy link
Member Author

You'd check out the branch and do a build (npm run build) to obtain a ZIP to use on your site. I've updated the description of the PR to include a ZIP for you: #3226

In any case, @cathyxz shared information with me about this component. Indeed, custom styling for amp-lightbox-gallery is not currently supported. You could add styles but they might break some behavior of the component in the future. So what you should do is file an issue to request the use case(s) you have for styling. For example, public CSS classes could be added to obvious elements that should allow for styling, like the description.

@austintude
Copy link

austintude commented Sep 11, 2019 via email

@westonruter
Copy link
Member Author

@austintude Please share the link to the issue on the amphtml repo.

@kienstra kienstra mentioned this issue Sep 17, 2019
Merged
@westonruter
Copy link
Member Author

westonruter commented Sep 18, 2019 •
edited
Loading

@rsmith4321 @waviaei @archon810 @austintude Please review #3285.

@westonruter
Copy link
Member Author

@rsmith4321 @waviaei @archon810 @austintude Please provide feedback on what your expectations are for this. See #3285 (comment).

@kienstra kienstra removed their assignment Oct 10, 2019
@westonruter westonruter added the P2 Low priority label Oct 24, 2019
@kienstra kienstra self-assigned this Oct 25, 2019
@kienstra kienstra assigned westonruter and unassigned kienstra Oct 25, 2019
@westonruter westonruter removed their assignment Oct 25, 2019
@westonruter westonruter added this to the v1.4 milestone Oct 28, 2019
@westonruter westonruter removed the P2 Low priority label Oct 28, 2019
@kienstra
Copy link
Contributor

Testing Steps

Hi @csossi,
Could you please test this, using the steps to test here?

Thanks, Cluadio!

@kienstra kienstra assigned csossi and unassigned kienstra Oct 28, 2019
@csossi
Copy link

csossi commented Oct 29, 2019

Verified in QA:

@csossi csossi removed their assignment Oct 29, 2019
@kienstra kienstra mentioned this issue Oct 29, 2019
Closed
@westonruter westonruter mentioned this issue Apr 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Enhancement New feature or improvement of an existing one
Projects
None yet
Milestone
v1.4
7 participants
@westonruter @archon810 @waviaei @kienstra @rsmith4321 @csossi @austintude