[Land and See - Stories Page] Next button doesn't auto scroll to top

[mattludwig]

Not sure if this is intended, but the next button at the bottom of the stories list doesn't auto scroll to the top of the page like I'd expect after clicking.

[afilbert]

The paging buttons on Stories page are using amp-bind with an amp-list to request different stories depending on the page. I'm not sure if there's an amphtml method for scrolling to page top, but it looks like it's been a requested feature for embeds: https://github.com/ampproject/amphtml/issues/7920

[afilbert]

I took a look at this just now and I was able to turn the button into an a anchor with href containing just an empty location hash: href="#". That "works" in that using the on action still works with amp-bind, but it definitely confuses amp-animation/amp-position-observer for next/prev pages containing a lot of items. This confusion is a consequence, I think, of the page jump.

That results in the amp-list CORS response completing and swapping out the content... but the animation either fails to fire (likely the case), or fires too early/late. Ultimately, the user needs to scroll down and then back up for the animation to work. The jump itself is jarring, too, especially when coupled with a delay in the CORS response.

I think a better solution would be to have an AMP-friendly way of using a script to ease the transition back to the top of the page. It's worth noting, too that this all may be the result of the paging implementation. From amp-list and amp-live-list, the preferred approach appears to be more of a "view more" approach which appends or reveals more items at the bottom of the initial results.

We could keep as is, or remove the land-see-pre-animate class from any stories resulting from paging. Since a user can page backward, however, to ...-page-1.json, we'd either need to remove all pre-animate states entirely from any paged results (which would make paged categories inconsistent with non-paged ones), or dynamically add/remove classes using amp-bind. Given different content types (video, img, amp-anim), and breakpoint classes, that would become more difficult to manually manage.

According to AMP by Example, classes are overridden when using amp-bind. If there was a way to append classes instead of completely overriding initial ones, it may be easier to selectively add/remove just the pre-animate class.

I also thought about moving the position observer to below the story categories nav, but I think it needs to stay in the amp-list template to work with category results.

For now, I'm going to keep as is but I'm open to any suggestions that I might be overlooking that might help simplify the animation approach.

[camelburrito]

https://github.com/ampproject/amphtml/blob/master/examples/standard-actions.amp.html#L74

Check if this helps you

[afilbert]

Thanks for the suggestion, @camelburrito. Was out most of the weekend prepping to move to new house, but I'll be able to take a look and try this evening. I'll push to #750 if all works well, or update here.

[afilbert]

Ok, I was able to test scrollTo with mixed results. It seems to be dependent upon a couple things:

1. The height of the page (I'm setting amp-state in the paging on action).
2. The amount of time the CORS request takes to complete.

I could increase the scroll time beyond the default half second, but I think that's kind of a half-measure.

I'm submitting an update for images to #750, but will leave the paging as is for now.

Correction: Didn't realize #750 was merged. New PR is #752.

[camelburrito]

@afilbert - using scroll to to a parent container should take you to the top of the container right? I think having the scroll to is better than not having it.

[afilbert]

@camelburrito that's correct. I was able to get it to scroll nicely using .scrollTo(). The challenge is that when it scrolls, particularly on mobile, sometimes the amp-animation events don't trigger. This ends up leaving the entire content region invisible to the user until they scroll down and then back up again. I reasoned that it was better to have content show up and make the user aware of it than auto-scroll and risk the content not showing up at all. An easy solution would be to remove slide-in animations from the Stories template content.

[camelburrito]

Can you file a bug on amphtml with links / steps to repro, and link that bug here

On Sun, Nov 26, 2017 at 7:21 PM Aron Filbert notifications@github.com wrote:

@camelburrito https://github.com/camelburrito that's correct. I was able to get it to scroll nicely using .scrollTo(). The challenge is that when it scrolls, particularly on mobile, sometimes the amp-animation events don't trigger. This ends up leaving the entire content region invisible to the user until they scroll down and then back up again. I reasoned that it was better to have content show up and make the user aware of it than auto-scroll and risk the content not showing up at all. An easy solution would be to remove slide-in animations from the Stories template content.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ampproject/ampstart/issues/728#issuecomment-347069174, or mute the thread https://github.com/notifications/unsubscribe-auth/AA_T_7O6OA0A3ZDMm0FJe41w7T5FwDTQks5s6irQgaJpZM4QQKQT .

-- -- Sriram

[afilbert]

Done. Here ya go, @camelburrito: https://github.com/ampproject/amphtml/issues/12260