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

[Gutenberg] Add AMP Carousel for Gallery and AMP Lightbox features for Gallery and Image #1121

Merged
merged 33 commits into from
May 30, 2018
Merged

[Gutenberg] Add AMP Carousel for Gallery and AMP Lightbox features for Gallery and Image #1121

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
52943ed
Start adding carousel toggle for Gallery.
miina May 4, 2018
fd662d7
Merge #1008.
miina May 5, 2018
54f9555
Add amp-carousel class.
miina May 5, 2018
ed1f9de
Filter saving the post for when AMP Plugin gets disabled. Add tests.
miina May 5, 2018
0263878
Add lightbox logic for Image block.
miina May 7, 2018
40486fe
Add lightbox for gallery block.
miina May 8, 2018
96bd515
Fix CSS.
miina May 8, 2018
553c4da
Merge develop.
miina May 8, 2018
22060ec
Add block-related JS with blocks filter instead of admin filter.
miina May 8, 2018
5b63476
Fix issue with assigning props directly.
miina May 9, 2018
2e102ea
Add lightbox to gallery shortcode block.
miina May 9, 2018
c3aad13
Merge remote-tracking branch 'origin/develop' into add/amp-carousel-t…
miina May 16, 2018
dd924ec
Merge #1026
miina May 16, 2018
ca133a4
Adjust logic to using data- attributes.
miina May 16, 2018
22597fa
Fixes. Merge #1008
miina May 21, 2018
b70af66
Update method comments. Remove unused dynamicBlocks data.
miina May 21, 2018
438567d
Merge branch 'develop' of https://github.com/Automattic/amp-wp into a…
westonruter May 21, 2018
7ee5fd3
Merge branch 'develop' into add/amp-carousel-toggle
westonruter May 23, 2018
58430e4
Merge develop & resolve conflicts.
miina May 25, 2018
55e8fff
Resolve conflicts.
miina May 25, 2018
f658c43
Merge remote-tracking branch 'origin/develop' into add/amp-carousel-t…
miina May 28, 2018
6900e2b
Fixes.
miina May 28, 2018
a69dd55
Merge develop & resolve conflicts.
miina May 30, 2018
ff2cdd2
Merge develop.
miina May 30, 2018
d5169ef
Fixes after merge.
miina May 30, 2018
13267f6
Fix displaying AMP carousel.
miina May 30, 2018
134335e
Fix checking for shortcode.
miina May 30, 2018
26c491e
Fix jscs.
miina May 30, 2018
46c3a50
Merge branch 'develop' of https://github.com/Automattic/amp-wp into a…
westonruter May 30, 2018
201654f
Fix AMP carousel for gallery block.
westonruter May 30, 2018
f9f3a05
Pass through all attributes from img to amp-img
westonruter May 30, 2018
cc53173
Force carousels to be used when AMP theme support is absent (for back…
westonruter May 30, 2018
dbe281c
Remove apparently unneessary object-fit from amp-image-lightbox; remo…
westonruter May 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions assets/css/amp-default.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,11 @@
/** Force the image into a box of fixed dimensions and use object-fit to scale. **/
object-fit: contain;
}

#amp-image-lightbox-1 img {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should be targeting img elements in selectors. I should think that only targeting amp-img should be done since img should really be the domain of the amp-img to control?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like amp-image-lightbox component is empty by default and clicking on image which is targeting this lightbox will insert the img element into the amp-image-lightbox, the amp-image-lightbox component doesn't include an additional amp-img component within, targeting amp-img doesn't seem to be possible in this case.
For example after clicking on an an image the lightbox HTML looks like this:
screen shot 2018-05-21 at 10 34 06 pm
Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to use AMP-generated class instead of img within 6900e2b.

object-fit: contain;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to be having any effect. I don't think it is needed?

}

.entry__content .wp-block-gallery[class*="columns-"] figure {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

entry__content is not a standard class name.

width: 100%;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this may just be needed in the ampnews theme. It doesn't seem necessary in Twenty Fifteen. It's probably just that the ampnews theme lacks all of the required styles.

}
Loading