-
Notifications
You must be signed in to change notification settings - Fork 383
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
Stop pinning amp-carousel at v0.1 so that v0.2 can be used #3115
Conversation
Would wait on us moving all carousels in amp.dev and manually confirming that before merging this to be safe |
9ad2c89
to
1b867d0
Compare
@nainar Any update on this? I'm looking at https://amp.dev/documentation/examples/components/amp-carousel/ and I'm still seeing v0.1: <script custom-element="amp-carousel" src="https://cdn.ampproject.org/v0/amp-carousel-0.1.js" async=""></script> |
1b867d0
to
4d2f864
Compare
@kienstra I think this is ready for review now, and that we should merge it for the |
And actually, Naina says it's ready to go, so once we've tested we'll make it part of v1.4.1. |
Question About Right Arrow In v0.2 Hi @nainar, Maybe this isn't an issue with AMP HTML, and there's some styling we could apply to prevent this. Here's a CodePen of it. On changing Steps to reproduce in WordPress
This isn't a common issue, and I didn't see it with most Core themes. Maybe there's some styling that Twenty Nineteen has that's associated with this issue. |
Actually, I should probably make the comment above into an issue for |
@kienstra I'm not seeing the same thing. For the first carousel, I see no arrow: And in the second I see no next arrow: |
Approved Hi @westonruter, |
I could replicate the issue: ampproject/amphtml#25510 (comment) |
@schlessera, thanks for testing this. I reopened the |
Upstream fix has been merged: ampproject/amphtml#25549 So we can merge this PR when that AMP update has been deployed, unless we're happy to live with some flaky buttons in |
Sounds good! |
…ental amp-base-carousel" This reverts commit c93cb0b.
4d2f864
to
873a4bf
Compare
The fix was released a couple days ago in https://github.com/ampproject/amphtml/releases/tag/1911191835190 |
verified in qa |
@westonruter Sorry to revive a necro post, but we're still seeing 0.1 load instead of 0.2. What could be the cause? |
@archon810 The issue is that amp-carousel 0.1 is being loaded by https://cdn.ampproject.org/v0/amp-lightbox-gallery-0.1.js, since v0.1 is still the |
@westonruter in the above post where the bug occurs we are just using lightbox gallery by adding |
@maciejmackowiak ok, but are you trying to fix a problem? Or are you just wondering why a different version is showing up versus what is expected? |
@westonruter From what I understand, we're trying to resolve the bugs that are associated with 0.1 and should be fixed in 0.2. Specifically
Judging by the fact that all of these were verified and closed before, 0.2 was live, but was then reverted back to 0.1 somehow? |
@archon810 No, the issue is that If you try adding a Gallery to a post and enable the carousel toggle in AMP Settings, then you'll see that the So, as a workaround to this I've opened #6509 which will allow you to manually |
@maciejmackowiak Please take note and implement the fix when the relevant AMP release is out. |
@maciejmackowiak @archon810 The PR has been merged and cherry-picked into the |
This is a follow-up to #3084.
It turns out that
amp-carousel
0.2 wasn't broken due to a dependence onamp-base-carousel
, but there was some other issue: ampproject/amphtml#23966. So we can allowamp-carousel
to update from 0.1 to 0.2 once the AMP fix is live.Fixes #3700.