[amp-carousel] Add support for defined number of loops to autoplay by nainar · Pull Request #18981 · ampproject/amphtml

nainar · 2018-10-26T01:43:14Z

Addresses #18911 for carousel V1 only.

honeybadgerdontcare · 2018-10-26T02:05:30Z

extensions/amp-carousel/validator-amp-carousel.protoascii

  attrs: {
    name: "autoplay"
    value: ""
+    value_regex: "[0-9]+"


For attrs it's considered a oneof of different types of values. Some can be repeated like value. In this case I'd recommend value_regex: "(|[0-9]+)" since it should allow both empty and some numeric.

For limited sets repeating value is sometimes easier to read than regex, e.g.:

value: "_top" value: "_blank"

instead of
value_regex: "(_top|_blank)"

Made the change @honeybadgerdontcare, also added a validator test for this.

…autoplay

aghassemi · 2018-10-29T16:39:20Z

extensions/amp-carousel/0.1/base-slides.js

    this.hasAutoplay_ = this.element.hasAttribute('autoplay');
+    const autoplayVal = this.element.getAttribute('autoplay');
+    if (autoplayVal != '') {
+      this.autoplayLoops_ = parseInt(autoplayVal, 10);


user().assert(isFiniteNumber(this.autoplayLoops_)) would be nice
(isFiniteNumber is in types.js)

aghassemi · 2018-10-29T16:42:33Z

extensions/amp-carousel/0.1/slidescroll.js

+    // we have reached the last slide in the set
+    // carry out removing autoplay logic.
+    // This only works because the initial Slide is always the first slide.
+    if (this.autoplayLoops_ && this.slideIndex_ === this.noOfSlides_ - 1) {


wha if autoplayLoops_ is set to 0? this logic would treat it as infinite loop. We probably want to either disallow it or treat autoplay=0 same as no autoplay

Done. If the value of autoplayLoops_ is set to 0 we never call setupAutoplay_ or autoplay_

aghassemi · 2018-10-29T16:44:30Z

extensions/amp-carousel/0.1/base-slides.js

+    this.clearAutoplay();
+    this.hasAutoplay_ = false;
+    this.shouldAutoplay_ = this.hasAutoplay_ && this.isLoopingEligible();
+    this.element.removeAttribute('loop');


we should not disable loop unless it was forced by autoplay in the first place. If author has loop specified, after autoplay ends, user should still manually loop.

Done. Put a guard around it that checks that autoplay is set as well.

nainar

@aghassemi made the changes you asked for. Added a test for autoplay=0 as well.

nainar · 2018-10-29T17:18:58Z

extensions/amp-carousel/0.1/base-slides.js

    this.hasAutoplay_ = this.element.hasAttribute('autoplay');
+    const autoplayVal = this.element.getAttribute('autoplay');
+    if (autoplayVal != '') {
+      this.autoplayLoops_ = parseInt(autoplayVal, 10);


nainar · 2018-10-29T17:19:18Z

extensions/amp-carousel/0.1/base-slides.js

+    this.clearAutoplay();
+    this.hasAutoplay_ = false;
+    this.shouldAutoplay_ = this.hasAutoplay_ && this.isLoopingEligible();
+    this.element.removeAttribute('loop');


Done. Put a guard around it that checks that autoplay is set as well.

nainar · 2018-10-29T18:00:57Z

extensions/amp-carousel/0.1/slidescroll.js

+    // we have reached the last slide in the set
+    // carry out removing autoplay logic.
+    // This only works because the initial Slide is always the first slide.
+    if (this.autoplayLoops_ && this.slideIndex_ === this.noOfSlides_ - 1) {


Done. If the value of autoplayLoops_ is set to 0 we never call setupAutoplay_ or autoplay_

aghassemi · 2018-10-29T18:18:03Z

extensions/amp-carousel/0.1/base-slides.js

+    this.shouldAutoplay_ = this.hasAutoplay_ && this.isLoopingEligible();
+    if (this.element.hasAttribute('autoplay')) {
+      // Only remove if specified due to the `autoplay` attribute
+      this.element.removeAttribute('loop');


this.hasLoop_ = false; this.shouldLoop = false;

too?

…tests

aghassemi · 2018-10-29T20:42:55Z

extensions/amp-carousel/0.1/base-slides.js

+    this.clearAutoplay();
+    this.hasAutoplay_ = false;
+    this.shouldAutoplay_ = this.hasAutoplay_ && this.isLoopingEligible();
+    if (this.element.hasAttribute('autoplay')) {


just the attribute check ignores the isLoopingEligible part. The value of this.shouldAutoplay_ before it changes should be the condition.

Agree with you there. Since we set this.hasAutoplay_ to false we are guaranteed that this.shouldAutoplay_ will be false. I can still guard it?

I guess we don't even get here if it was not eligible for autoplay. I think the check to remove loop only if it was added because of autoplay is still wrong. Maybe it should be

// loop added due to autoplay if element originally did not have `loop` attribute, so remove it. if(!this.element.hasAttribute('loop') { ... }

looking at this again, I realize with don't actually have a signal to know whether loop was added because of autoplay or it was there from the beginning. We need a new variable to track that when autoplay adds loop.

We can also just maintain state to see if loop was added by autoplay status addition

Done in latest Patch - 1b9d553

aghassemi

Left one comment, feel free to merge after addressing.

…autoplay

nainar · 2018-11-01T19:47:18Z

@aghassemi made the change in this patch: a247fe3

Will merge if it looks good to you.

…autoplay

aghassemi · 2018-11-02T17:40:10Z

LGTM

* cl/219531121 Revision bump for #17481 * cl/219850294 Allow `http` scheme for links in email spec * cl/219866890 Revision bump for #19043 * cl/219867113 Revision bump for #18981 * cl/219877087 Make ESlint happy. * cl/219882876 Fix lint for #19096

…mpproject#18981)

* cl/219531121 Revision bump for ampproject#17481 * cl/219850294 Allow `http` scheme for links in email spec * cl/219866890 Revision bump for ampproject#19043 * cl/219867113 Revision bump for ampproject#18981 * cl/219877087 Make ESlint happy. * cl/219882876 Fix lint for ampproject#19096

[amp-carousel] Add support for defined number of loops to autoplay

7d8fd6f

googlebot added the cla: yes label Oct 26, 2018

nainar requested a review from aghassemi October 26, 2018 01:47

honeybadgerdontcare reviewed Oct 26, 2018

View reviewed changes

nainar added 3 commits October 26, 2018 09:39

Merge branch 'master' of github.com:ampproject/amphtml into carousel_…

4965b04

…autoplay

Added validator changes

08bc239

Add documentation

0f9b7dd

aghassemi reviewed Oct 29, 2018

View reviewed changes

nainar commented Oct 29, 2018

View reviewed changes

aghassemi reviewed Oct 29, 2018

View reviewed changes

Only remove loop if loop if autoplay, guard for autoplay = 0 and add …

e4f0255

…tests

nainar force-pushed the carousel_autoplay branch from d84ee9f to e4f0255 Compare October 29, 2018 20:06

aghassemi reviewed Oct 29, 2018

View reviewed changes

aghassemi approved these changes Nov 1, 2018

View reviewed changes

nainar added 2 commits November 1, 2018 10:26

Merge branch 'master' of github.com:ampproject/amphtml into carousel_…

5cde693

…autoplay

Fix up logic for removing looping

a247fe3

nainar added 2 commits November 1, 2018 16:36

Merge branch 'master' of github.com:ampproject/amphtml into carousel_…

519cbc0

…autoplay

Maintain loop being added state

1b9d553

aghassemi merged commit a0ca29d into ampproject:master Nov 2, 2018

twifkak added a commit to twifkak/amphtml that referenced this pull request Nov 2, 2018

cl/219867113 Revision bump for ampproject#18981

5b0603a

twifkak added a commit to twifkak/amphtml that referenced this pull request Nov 2, 2018

cl/219867113 Revision bump for ampproject#18981

6fe9313

twifkak mentioned this pull request Nov 2, 2018

Validator rollup #19120

Merged

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels Nov 14, 2018

Enriqe pushed a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018

✨ [amp-carousel] Add support for defined number of loops to autoplay (a…

5fd9433

…mpproject#18981)

sparhami mentioned this pull request Feb 14, 2019

[carousel v2] Support a maximum number of auto advance loops. #20872

Merged

Conversation

nainar commented Oct 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nainar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aghassemi left a comment

Choose a reason for hiding this comment

Uh oh!

nainar commented Nov 1, 2018

Uh oh!

aghassemi commented Nov 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants