Add goToSlide to scrollable-slides by cathyxz · Pull Request #14477 · ampproject/amphtml

cathyxz · 2018-04-06T21:41:08Z

Closes #13011.
My only question here is does it make sense to scroll so that the target slide is in the center of the carousel or the left? The way I interpreted the aesthetic of the scrollable-carousel, was that the target slide was the left-most slide. But maybe I'm misinterpreting how people use them.

cathyxz · 2018-04-06T22:22:35Z

extensions/amp-carousel/0.1/scrollable-carousel.js

+   * Calculates the target scroll position for the given slide index.
+   * @param {number} index
+   */
+  getPosForSlideIndex_(index) {


Right now this implementation tries to center the target slide. If we remove the latest commit, it will try to left-align the target slide.

I think center makes sense. we can revisit if there is interest in other docking locations.

cvializ · 2018-04-09T17:00:04Z

extensions/amp-carousel/0.1/scrollable-carousel.js


+  /**
+   * Scrolls to the slide at the given slide index.
+   * @param {*} value


nit: I would move the parseInt into the registerAction callback, that way this function can declare explicitly that it needs a number.

cvializ · 2018-04-09T17:04:19Z

extensions/amp-carousel/0.1/scrollable-carousel.js

+    this.registerAction('goToSlide', invocation => {
+      const args = invocation.args;
+      if (args) {
+        this.goToSlide_(args['index']);


Do you think this action should be chainable? Actions can return a Promise, and other subsequent actions can be made to wait for that Promise to resolve. I can see a publisher wanting to go to a slide, then do something after the carousel has finished animating to that slide. wdyt?

Excellent idea.

tbh, I am not a big fan of our Promise-based actions since author is not in control of whether they want to chain or parallelize the actions. slidescroll version has a slideChange event. I suggest supporting that event here as well (if not supported already, not sure if it is already coded in the base class or not). With that event, authors can react after a slidechage is fully done.

I see. Okay. =)

So with regards to supporting the slideChange event, in the case where the user is manually scrolling, it's not actually obvious which slide index the carousel is on. Per offline discussion, I'll trigger this event when the action is called, and leave an issue to define slide index in context of scrolling.

Actually, per doc, we shouldn't trigger the event when the action is called:

Fired when the user manually changes the carousel's current slide. Does not fire on autoplay or the goToSlide action.

I'll file an issue for later.

Documentation might be wrong, based on the code, I see it get fired with gotToSlide action.

Filed: #14505 to avoid getting too greedy. Stacked the PR though.

cvializ · 2018-04-09T17:08:27Z

For the center vs left question, @aghassemi might have better context than I do, but I think center makes sense. It's an easy change if someone opens a bug.

aghassemi · 2018-04-09T17:13:34Z

extensions/amp-carousel/0.1/scrollable-carousel.js

+   * Calculates the target scroll position for the given slide index.
+   * @param {number} index
+   */
+  getPosForSlideIndex_(index) {


I think center makes sense. we can revisit if there is interest in other docking locations.

aghassemi · 2018-04-09T17:15:18Z

extensions/amp-carousel/0.1/scrollable-carousel.js

+   * @private
+   */
+  goToSlide_(value) {
+    const index = parseInt(value, 10);


whole function should become a measure/mutate, where getPosForSlideIndex_ is called in the measure block and start of animation is in the mutate block.

jridgewell · 2018-04-09T22:06:40Z

extensions/amp-carousel/0.1/scrollable-carousel.js

+        const index = parseInt(args['index'], 10);
+        this.goToSlide_(index);
+      }
+    }, ActionTrust.HIGH);


Is high trust needed?

I followed the precedence in slidescroll.js here in terms of ActionTrust. I'm not sure why this is high trust, but I imagine that the trust level should be consistent across the two carousel implementations.

Both can become low trust given the UI changes are fully contained within the container.

Actually I think the reason this is high trust is because we allow amp-bind for the [slide] attribute, which also must update when change the slide (no matter how). I reverted the trust change on type="slides". We can keep it low trust on type="carousel" until we implement the slideChange event and other support for amp-bind.

jridgewell · 2018-04-09T22:10:29Z

extensions/amp-carousel/0.1/scrollable-carousel.js

+      });
+    };
+
+    this.vsync_.run({


VSync will be a deprecated API soon (I'm actively removing every use). Please use BaseElement#measureMutateElement, and note that state is not supported (it doesn't help you in the this situation anyways).

That's good to know! Could we get a wider distribution on this? I think the currently known best practice is still to use vsync for measurements and mutations.

Added to tomorrow's deep dive.

jridgewell · 2018-04-09T22:11:06Z

extensions/amp-carousel/0.1/scrollable-carousel.js

+   * @param {number} index
+   * @private
+   */
+  goToSlide_(index) {


Can this be used anywhere else?

jridgewell · 2018-04-09T22:45:59Z

extensions/amp-carousel/0.1/scrollable-carousel.js

-      mutate: mutateSlidePosition,
-    }, {});
+    this.measureElement(() => this.getPosForSlideIndex_(index))
+        .then(newPos => {


Note: measure.then.mutate is to take two vsync cycles (~16ms-32ms lag) to complete. You can combine them into a single #measureMutateElement call, which will complete in 1 cycle.

Will that guarantee that the measure finishes before the mutate? Implemented the change assuming that the above is true.

cathyxz · 2018-04-09T23:51:05Z

@jridgewell PTAL?

jridgewell

Defer to Ali.

cathyxz · 2018-04-10T01:08:47Z

The failing test seems legitimate. Investigating.

aghassemi · 2018-04-10T16:45:59Z

@cathyxz In the previous commit, trust level of the slidechange "event" was changed to low. We want to keep the events high trust, just lower the goToSlide "action" on slidescroll to low trust.

aghassemi · 2018-04-10T17:23:34Z

* Add goToSlide to scrollable-slides * Try to center the target slide * Wrap everything in vsync * Remove vsync, set action trust to LOW * Use measureMutateElement for shorter cycle * Revert ActionTrust change on slidescroll * Change goToSlide action to low trust

Add goToSlide to scrollable-slides

7185d62

googlebot added the cla: yes label Apr 6, 2018

cathyxz commented Apr 6, 2018

View reviewed changes

cathyxz requested review from aghassemi, cvializ and jridgewell April 6, 2018 22:22

Try to center the target slide

03a8466

cathyxz force-pushed the feature/carousel-go-to-slide branch from 65e44fa to 03a8466 Compare April 6, 2018 23:36

cvializ reviewed Apr 9, 2018

View reviewed changes

aghassemi reviewed Apr 9, 2018

View reviewed changes

Wrap everything in vsync

9c51276

cathyxz mentioned this pull request Apr 9, 2018

Add slideChange event to amp-scrollable-carousel #14505

Closed

aghassemi approved these changes Apr 9, 2018

View reviewed changes

jridgewell requested changes Apr 9, 2018

View reviewed changes

Remove vsync, set action trust to LOW

9927e16

jridgewell reviewed Apr 9, 2018

View reviewed changes

Use measureMutateElement for shorter cycle

27cf19c

cathyxz force-pushed the feature/carousel-go-to-slide branch from 158c1dc to 27cf19c Compare April 9, 2018 22:59

jridgewell approved these changes Apr 10, 2018

View reviewed changes

Revert ActionTrust change on slidescroll

0df9919

Change goToSlide action to low trust

28def64

cathyxz merged commit db29e31 into ampproject:master Apr 10, 2018

cathyxz deleted the feature/carousel-go-to-slide branch April 10, 2018 18:58

This was referenced Apr 16, 2018

Status Update 2018-04-02 #14329

Closed

Status Update 2018-04-16 #14616

Closed

cathyxz mentioned this pull request Jun 4, 2018

Error: Attempting to access a non-existant slide #15772

Closed

Conversation

cathyxz commented Apr 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

cathyxz Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

cvializ commented Apr 9, 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!

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!

cathyxz Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cathyxz commented Apr 9, 2018

Uh oh!

jridgewell left a comment

Choose a reason for hiding this comment

Uh oh!

cathyxz commented Apr 10, 2018

Uh oh!

aghassemi commented Apr 10, 2018

Uh oh!

aghassemi commented Apr 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

cathyxz commented Apr 6, 2018 •

edited

Loading

cathyxz Apr 9, 2018 •

edited

Loading

cathyxz Apr 9, 2018 •

edited

Loading