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.

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

Classical amp-inline-gallery-captions (0.1) #32075

Merged
merged 10 commits into from Feb 24, 2021

Conversation

caroqliu
Copy link
Contributor

@caroqliu caroqliu commented Jan 20, 2021

This PR introduces #26076 into the codebase under experimental amp-inline-gallery-captions flag with minimal changes, primarily to check in the WIP as a baseline to jump off of going forward.

Demo

*/

amp-inline-gallery-captions {
pointer-events: none;
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit surprised by it. Does it mean we are actually not allowing any links in captions? How do "more" and "less" buttons work?

Copy link
Contributor Author

@caroqliu caroqliu Feb 11, 2021

Choose a reason for hiding this comment

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

It's because this implementation doesn't actually have any interactivity or interactive descendants in the amp-inline-gallery-captions element, which is effectively just a fixed height block of space that gets covered by captions elements in a different tree (amp-inline-gallery-slide). Agreed it is a bit unconventional and likely not necessary, but it should not be harmful.

<amp-inline-gallery layout="container">
    <amp-base-carousel>
      <amp-inline-gallery-slide>
        <amp-img></amp-img>
        <!-- clickable and slotted into an amp-truncate-text element added to amp-inline-gallery-slide -->
        <span class="caption" slot="caption"> 
          The first slide's caption.
        </span>
        <span class="attribution" slot="attribution">This is an attribution (not truncated).</span>
      </amp-inline-gallery-slide>
      ... 
    </amp-base-carousel>
    <amp-inline-gallery-captions layout="fixed-height" height="3.75em">
    </amp-inline-gallery-captions>
  </amp-inline-gallery>

* @param {number} total
* @param {number} index
* @param {number} offset
*/
updateProgress(total, index, offset) {
updateProgress(slides, total, index, offset) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for?

Copy link
Contributor Author

@caroqliu caroqliu Feb 11, 2021

Choose a reason for hiding this comment

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

It's to match with this part, which based on CHILDREN_FOR_PROGRESS_SELECTOR calls updateProgress for both -pagination and -captions:

updateProgress_(slides, total, index, offset) {
iterateCursor(
scopedQuerySelectorAll(this.element, CHILDREN_FOR_PROGRESS_SELECTOR),
(el) => {
el.getImpl().then((pagination) => {
pagination.updateProgress(slides, total, index, offset);
});
}
);

We can either move the unneeded slides to the end for pagination or separate it out -- for now I've done the former:
Unused:

Suggested change
updateProgress(slides, total, index, offset) {
updateProgress(total, index, offset, unusedSlides) {

Separate:

  updateProgress_(total, index, offset) {
    iterateCursor(
      scopedQuerySelectorAll(this.element, PAGINATION_SELECTOR),
      (el) => {
        el.getImpl().then((pagination) => {
          pagination.updateProgress(total, index, offset);
        });
      }
    );

    iterateCursor(
      scopedQuerySelectorAll(this.element, CAPTION_SELECTOR),
      (el) => {
        el.getImpl().then((caption) => {
          caption.updateProgress(slides, total, index, offset);
        });
      }
    );
  }

@caroqliu caroqliu changed the title [draft] amp-inline-gallery-captions Classical amp-inline-gallery-captions (0.1) Feb 11, 2021
@caroqliu caroqliu marked this pull request as ready for review February 23, 2021 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants