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

[amp-story-player] Opt-out of animation in go() / show() #30889

Closed
Enriqe opened this issue Oct 27, 2020 · 4 comments · Fixed by #34204
Closed

[amp-story-player] Opt-out of animation in go() / show() #30889

Enriqe opened this issue Oct 27, 2020 · 4 comments · Fixed by #34204

Comments

@Enriqe
Copy link
Contributor

Enriqe commented Oct 27, 2020

Currently using the go() (or show(), since it uses go() internally) will show the swiping animation. Sometimes this is undesirable and would be ideal to opt-out of it. e.g. a carousel entry point.

@ampproject/wg-stories

@Enriqe Enriqe self-assigned this Oct 27, 2020
@Enriqe Enriqe changed the title [amp-story-player] Opt-out of animation in go() [amp-story-player] Opt-out of animation in go() / show() Feb 4, 2021
@mszylkowski mszylkowski self-assigned this May 3, 2021
@mszylkowski
Copy link
Contributor

What we need:

  • Publishers that currently implement navigation buttons for next and previous should not have their animation removed. They use go(1) and go(-1)
  • When entrypoints open the player with a user clicked story, they will call show and it should not be animated.
  • Pages that are further away can be displayed with go(5), but it doesn't make sense to animate these since they are not even loaded, and this use-case is not the norm.
  • Auto-advancing or user swiping should always animate

A logic that could work by default is: "when a user script programmatically calls show, we show the story without animating. When a user script programmatically calls go with a parameter between -1 and 1, we animate the story, but if the parameter is out of bounds, we don't".

Then, we can have a config behavior.programmaticNavigationAnimation, which can be set to true to animate all user script calls (even for .show calls), or set to false to disable animations (even for go calls).

@ampproject/wg-stories wdyt?

@gmajoulet
Copy link
Contributor

Having some level of magic behind a parameter makes it very hard for a developer to predict its behavior.

Having a second (or third I forgot) parameter like go(offset, options = {animate: bool}) sounds much more easy to understand and use.

@Enriqe
Copy link
Contributor Author

Enriqe commented May 3, 2021

A logic that could work by default is: "when a user script programmatically calls show, we show the story without animating. When a user script programmatically calls go with a parameter between -1 and 1, we animate the story, but if the parameter is out of bounds, we don't".

There are some edge cases where this wouldn't work (e.g. with circular-wrapping and calling go(2) on story 2 of 3).

Publishers that currently implement navigation buttons for next and previous should not have their animation removed. They use go(1) and go(-1)

Unfortunately we can't know for sure if they are using go(), they could also be using show().

Auto-advancing or user swiping should always animate

👍🏽

I also think having mixed logic across different methods would confuse developers. I think we should get rid of the animation when calling go() or show() by default and only use them with the opt-in.

go(storyDelta, pageDelta, options = {animate}) / show(storyUrl, pageId, options = {animate}) SGTM.

@mszylkowski
Copy link
Contributor

This feature request doesn't necessarily need to change the defaults, but providing the options in the API SGTM. Seems like the player currently animates only when navigating to a story that is already built, which means it's only navigating for +1 and -1. Is this problematic in any way? To me this current default makes sense and if we provide users with the API way of configuring whether to show the animation, then this will be solved. Developers can decide based on what button the user clicked, what the behavior is.

Eg:

// Clicking on entrypoint
player.show(e.target.getAttribute("data-story"), null, {animate: false});
// Clicking on next button
player.go(1, null, {animate: false});

I also think having mixed logic across different methods would confuse developers. I think we should get rid of the animation when calling go() or show() by default and only use them with the opt-in.

This could negatively impact navigation for players that have a next and previous button, which is a primary use-case of the programmatic controls. My take on this is that we shouldn't remove these animations by default.

Unfortunately we can't know for sure if they are using go(), they could also be using show().

This can be solved with code changes so not a problem. However, changing the defaults for one method and not the other sounds confusing as you said.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants