search

ampproject / amp-wp

Enable AMP on your WordPress site, the WordPress way.
https://wordpress.org/plugins/amp/
GNU General Public License v2.0
1.79k stars 383 forks source link

Add image captions to the gallery carousel #2855

Closed westonruter closed 5 years ago

westonruter commented 5 years ago

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?

rsmith4321 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Also agreed with how this should work.

austintude commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Just trying to make the same modifications mentioned in my comment. But though the mods are being exported to my global.min.css file which gets imported into the <style amp-custom> there is no evidence of the aforementioned styles/mods on the published page

On Tue, Sep 10, 2019, 4:44 PM Weston Ruter notifications@github.com wrote:

What style rules are you adding to the page?

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

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ampproject/amp-wp/issues/2855?email_source=notifications&email_token=AHC4FRIOFPWRS24CDM4JDKLQJAILJA5CNFSM4IFMPMAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6MS5SQ#issuecomment-530132682, or mute the thread https://github.com/notifications/unsubscribe-auth/AHC4FRMV2HBPK5NEEQXBM43QJAILJANCNFSM4IFMPMAA .

westonruter commented 5 years ago

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 commented 5 years ago

Hm... ok. It appears that the selector #amp-lightbox-gallery is the injected (?) id into the <amp-lightbox-gallery> element when (if?) the amp-img is clicked (triggered with an on-click. BUT this element does not... exist(?) on the page until it has been processed. So tree-shake could think the element isn't on the page and therefore shake out the style? Just a thought.

On Tue, Sep 10, 2019, 5:05 PM Weston Ruter notifications@github.com wrote:

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.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ampproject/amp-wp/issues/2855?email_source=notifications&email_token=AHC4FRM3WIK7SENQK3QZK63QJAK3VA5CNFSM4IFMPMAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6MUOCI#issuecomment-530138889, or mute the thread https://github.com/notifications/unsubscribe-auth/AHC4FRLNXITWFYFJVUNKZOTQJAK3VANCNFSM4IFMPMAA .

westonruter commented 5 years ago

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 https://github.com/ampproject/amp-wp/pull/3226 which fixes this issue, but it's not clear yet whether the component should be styled directly.

austintude commented 5 years ago

Not sure how to integrate the PR in my current site's use of the plugin. Please advise. Thank you

On Tue, Sep 10, 2019, 7:02 PM Weston Ruter notifications@github.com wrote:

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 https://github.com/ampproject/amp-wp/pull/3226 which fixes this issue, but it's not clear yet whether the component should be styled directly.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ampproject/amp-wp/issues/2855?email_source=notifications&email_token=AHC4FROYVVVWRFCHXBYAANDQJAYQBA5CNFSM4IFMPMAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6M22QQ#issuecomment-530165058, or mute the thread https://github.com/notifications/unsubscribe-auth/AHC4FRK6HTEXV5V4KOQM2RLQJAYQBANCNFSM4IFMPMAA .

westonruter commented 5 years ago

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: https://github.com/ampproject/amp-wp/pull/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 commented 5 years ago

Thank you. I will file an issue as you recommend and go from there. Currently, it is a 'want' from my clients' sites not yet a 'need', so don't yet wish to push the envelope further than intended. However, I do believe my clients' desires are valid and warrant that kind of control so fingers crossed this kind of control will be supported soon!

Cheers for all that you do.

Daniel

On Wed, Sep 11, 2019 at 1:55 PM Weston Ruter notifications@github.com wrote:

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 https://github.com/ampproject/amp-wp/pull/3226

In any case, @cathyxz https://github.com/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 https://github.com/ampproject/amphtml/issues/new?assignees=&labels=Type%3A+Feature+Request&template=feature_request.md&title= 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.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ampproject/amp-wp/issues/2855?email_source=notifications&email_token=AHC4FRPZVRV6OM4KXHN2INDQJE5LRA5CNFSM4IFMPMAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6PQ4ZQ#issuecomment-530517606, or mute the thread https://github.com/notifications/unsubscribe-auth/AHC4FRL5ZIIS5RK47Q3T7R3QJE5LRANCNFSM4IFMPMAA .

--

Daniel Bisett Owner & CEO Austintatious Design https://austintatiousdesign.co/

Connect with me on:

https://www.facebook.com/austintude https://www.linkedin.com/in/austintude https://twitter.com/austintude https://www.youtube.com/channel/UCWOEzhwo7rUfg_Re8TRvArg

westonruter commented 5 years ago

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

westonruter commented 5 years ago

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

westonruter commented 5 years ago

@rsmith4321 @waviaei @archon810 @austintude Please provide feedback on what your expectations are for this. See https://github.com/ampproject/amp-wp/pull/3285#issuecomment-534242681.

kienstra commented 5 years ago

Testing Steps

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

Thanks, Cluadio!

csossi commented 5 years ago

Verified in QA: