Skip to content

amp-inline-gallery docs and validation rules#26109

Closed
sparhami wants to merge 2 commits intoampproject:masterfrom
sparhami:aig-docs
Closed

amp-inline-gallery docs and validation rules#26109
sparhami wants to merge 2 commits intoampproject:masterfrom
sparhami:aig-docs

Conversation

@sparhami
Copy link
Copy Markdown

@sparhami sparhami commented Dec 19, 2019

  • Add docs for amp-inline-gallery elements and linking to amp-base-carousel for its usage.
  • Add validation rules for amp-inline-gallery elements.
  • Fix selector for finding the amp-base-carousel, when it is a direct child of the amp-inline-gallery.

@sparhami sparhami requested a review from wassgha December 19, 2019 21:52
@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Dec 19, 2019 •
edited
Loading

Hey @ampproject/wg-caching, these files were changed:

  • extensions/amp-inline-gallery/0.1/test/validator-amp-inline-gallery-pagination.html
  • extensions/amp-inline-gallery/0.1/test/validator-amp-inline-gallery-pagination.out
  • extensions/amp-inline-gallery/0.1/test/validator-amp-inline-gallery-thumbnails.html
  • extensions/amp-inline-gallery/0.1/test/validator-amp-inline-gallery-thumbnails.out
  • extensions/amp-inline-gallery/validator-amp-inline-gallery.protoascii

honeybadgerdontcare
honeybadgerdontcare reviewed Dec 19, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

some minor changes, but otherwise looks good for validation

CrystalOnScript
CrystalOnScript requested changes Dec 19, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@CrystalOnScript CrystalOnScript left a comment

Choose a reason for hiding this comment

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

Some large restructuring requests for this one. Let me know if you have any questions :)

extensions/amp-inline-gallery/amp-inline-gallery.md
The above example shows slides using an aspect ratio of 3:2, with 10% of a slide peeking on either side. Note that the carousel itself uses an aspect ratio of 3.6:2 since we show 1.2 slides at a time.

### Using pagination indicators

Copy link
Copy Markdown
Contributor

@CrystalOnScript CrystalOnScript Dec 19, 2019

Choose a reason for hiding this comment

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

Insert a section above here:
"Display pagination indicators by including the <amp-inline-gallery-pagination> element within the <amp-inline-gallery"> component."

Small code sample here please :)

extensions/amp-inline-gallery/amp-inline-gallery.md
Where `media` is a [CSS Media Query](https://developer.mozilla.org/en-US/docs/Web/CSS/Media_Queries/Using_media_queries), which controls which pagination configuration to display.

### Using thumbnails

Copy link
Copy Markdown
Contributor

@CrystalOnScript CrystalOnScript Dec 19, 2019

Choose a reason for hiding this comment

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

Insert a section above here:
"Display thumbnails by including the <amp-inline-gallery-thumbnails> element within the <amp-inline-gallery"> component."

Small code sample here please :)

@googlebot googlebot added cla: no and removed cla: yes labels Jan 3, 2020
@googlebot
Copy link
Copy Markdown

googlebot commented Jan 3, 2020

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Jan 3, 2020
@ampproject ampproject deleted a comment from googlebot Jan 3, 2020
honeybadgerdontcare
honeybadgerdontcare reviewed Jan 3, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

nit: it's 2020 now, copyright could probably be updated ;)

@googlebot
Copy link
Copy Markdown

googlebot commented Feb 4, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Feb 4, 2020
@alanorozco alanorozco removed the cla: no label Feb 4, 2020
@alanorozco alanorozco mentioned this pull request Feb 10, 2020
Merged
@alanorozco
Copy link
Copy Markdown
Member

alanorozco commented Feb 10, 2020

Closing in favor of #26708, since CLA signing broke when making changes necessary for merge.

@alanorozco alanorozco closed this Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@honeybadgerdontcare honeybadgerdontcare honeybadgerdontcare left review comments

@CrystalOnScript CrystalOnScript CrystalOnScript requested changes

@wassgha wassgha Awaiting requested review from wassgha

Assignees

No one assigned

Labels

cla: no

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sparhami @googlebot @alanorozco @honeybadgerdontcare @CrystalOnScript