-
Notifications
You must be signed in to change notification settings - Fork 384
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
Fix lightbox effect if images are aligned #7178
Conversation
Plugin builds for 1131c4d are ready 🛎️!
|
For a left-aligned block with lightbox added… Block markup in Twenty Twenty-Two (FSE theme) where lightbox works: <figure data-amp-lightbox="true" class="wp-block-image alignleft size-full amp-wp-d9a41c3" data-amp-original-style="border-radius:0px"><amp-img width="384" height="384" src="http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1.png" alt="" class="wp-image-10 amp-wp-enforced-sizes i-amphtml-layout-intrinsic i-amphtml-layout-size-defined" srcset="http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1.png 384w, http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1-300x300.png 300w, http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1-150x150.png 150w" sizes="(max-width: 384px) 100vw, 384px" data-amp-lightbox="" lightbox="" layout="intrinsic" disable-inline-width="" data-hero-candidate="" data-hero i-amphtml-ssr i-amphtml-layout="intrinsic"><i-amphtml-sizer slot="i-amphtml-svc" class="i-amphtml-sizer"><img alt="" aria-hidden="true" class="i-amphtml-intrinsic-sizer" role="presentation" src="data:image/svg+xml;base64,PHN2ZyBoZWlnaHQ9IjM4NCIgd2lkdGg9IjM4NCIgeG1sbnM9Imh0dHA6Ly93d3cudzMub3JnLzIwMDAvc3ZnIiB2ZXJzaW9uPSIxLjEiLz4="></i-amphtml-sizer><img class="i-amphtml-fill-content i-amphtml-replaced-content" decoding="async" loading="lazy" alt="" src="http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1.png" srcset="http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1.png 384w, http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1-300x300.png 300w, http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1-150x150.png 150w" sizes="(max-width: 384px) 100vw, 384px"></amp-img></figure> Block markup in Twenty Twenty-One (non-FSE theme) where lightbox fails: <div class="wp-block-image">
<figure data-amp-lightbox="true" class="alignleft size-full amp-wp-d9a41c3" data-amp-original-style="border-radius:0px"><amp-img width="384" height="384" src="http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1.png" alt="" class="wp-image-10 amp-wp-enforced-sizes i-amphtml-layout-intrinsic i-amphtml-layout-size-defined" srcset="http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1.png 384w, http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1-300x300.png 300w, http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1-150x150.png 150w" sizes="(max-width: 384px) 100vw, 384px" layout="intrinsic" disable-inline-width="" data-hero-candidate="" data-hero i-amphtml-ssr i-amphtml-layout="intrinsic"><i-amphtml-sizer slot="i-amphtml-svc" class="i-amphtml-sizer"><img alt="" aria-hidden="true" class="i-amphtml-intrinsic-sizer" role="presentation" src="data:image/svg+xml;base64,PHN2ZyBoZWlnaHQ9IjM4NCIgd2lkdGg9IjM4NCIgeG1sbnM9Imh0dHA6Ly93d3cudzMub3JnLzIwMDAvc3ZnIiB2ZXJzaW9uPSIxLjEiLz4="></i-amphtml-sizer><img class="i-amphtml-fill-content i-amphtml-replaced-content" decoding="async" alt="" src="http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1.png" srcset="http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1.png 384w, http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1-300x300.png 300w, http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1-150x150.png 150w" sizes="(max-width: 384px) 100vw, 384px"></amp-img></figure>
</div> With the changes in this PR, the non-FSE markup becomes: <div class="wp-block-image">
<figure data-amp-lightbox="true" class="alignleft size-full amp-wp-d9a41c3" data-amp-original-style="border-radius:0px"><amp-img width="384" height="384" src="http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1.png" alt="" class="wp-image-10 amp-wp-enforced-sizes i-amphtml-layout-intrinsic i-amphtml-layout-size-defined" srcset="http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1.png 384w, http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1-300x300.png 300w, http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1-150x150.png 150w" sizes="(max-width: 384px) 100vw, 384px" data-amp-lightbox="" lightbox="" layout="intrinsic" disable-inline-width="" data-hero-candidate="" data-hero i-amphtml-ssr i-amphtml-layout="intrinsic"><i-amphtml-sizer slot="i-amphtml-svc" class="i-amphtml-sizer"><img alt="" aria-hidden="true" class="i-amphtml-intrinsic-sizer" role="presentation" src="data:image/svg+xml;base64,PHN2ZyBoZWlnaHQ9IjM4NCIgd2lkdGg9IjM4NCIgeG1sbnM9Imh0dHA6Ly93d3cudzMub3JnLzIwMDAvc3ZnIiB2ZXJzaW9uPSIxLjEiLz4="></i-amphtml-sizer><img class="i-amphtml-fill-content i-amphtml-replaced-content" decoding="async" alt="" src="http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1.png" srcset="http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1.png 384w, http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1-300x300.png 300w, http://localhost:8888/wp-content/uploads/2022/06/amp-logo-icon-512x512-1-150x150.png 150w" sizes="(max-width: 384px) 100vw, 384px"></amp-img></figure></div> And lightbox works. It also still works in an FSE theme. |
@westonruter Is this PR good to merge? If needed any changes please LMK. |
I haven't had a chance to finish reviewing yet. Sorry for the delay. |
18727d2
to
7d56e06
Compare
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
7d56e06
to
3e3dfdf
Compare
Thanks for your persistence with this. |
Summary
Fix the lightbox effect when the images are aligned. This is due to the adding of
data-amp-lightbox="true"
to the<figure>
element instead of adding it in<div>
.Also improved the logic of determining
media
URL and replacing it for lightbox effect.Fixes #7154
Checklist