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

Allowing exceptions to elementNearBottom resizing requirement #21016

Closed
cathyxz opened this issue Feb 22, 2019 · 7 comments
Closed

Allowing exceptions to elementNearBottom resizing requirement #21016

cathyxz opened this issue Feb 22, 2019 · 7 comments

Comments

@cathyxz
Copy link
Contributor

cathyxz commented Feb 22, 2019

TLDR: can we have exceptions to the no-resizing + show overflow rule here?

/**
* Returns true if element is within 15% and 1000px of document bottom.
* Caller can provide current/initial layout boxes as an optimization.
* @param {!./resource.Resource} resource
* @param {!../layout-rect.LayoutRectDef=} opt_layoutBox
* @param {!../layout-rect.LayoutRectDef=} opt_initialLayoutBox
* @return {boolean}
* @private
*/
elementNearBottom_(resource, opt_layoutBox, opt_initialLayoutBox) {
const contentHeight = this.viewport_.getContentHeight();
const threshold = Math.max(contentHeight * 0.85, contentHeight - 1000);
const box = opt_layoutBox || resource.getLayoutBox();
const initialBox = opt_initialLayoutBox || resource.getInitialLayoutBox();
return box.bottom >= threshold || initialBox.bottom >= threshold;
}

I have what I think is a legitimate use case here based on #14060 (comment) from infinite-scroll, where even though there is valid content in the footer and the bottom of a particular element is in a page, we would still like the element to resize.

sample

As shown in the image, infinite-scroll triggers when the user scrolls to 3 viewports away from the bottom of the list. But due to reasons like slow endpoint or network connection, it is a common occurrence that content of the next fetch has not yet arrived (hence the loading icon is seen) by the time the user reaches the bottom of the page. The current behaviour is that when the fetch comes back with items, we trigger overflow because the bottom of the list is not within 15% of the page bottom. The desired behaviour is that we resize successfully and continue on scrolling.

This is not a problem when there is nothing below the list. However, in some cases, it's possible to have footers of arbitrary length (e.g the business requirement to show an arbitrary number of search terms). I'm wondering can we either create exceptions to the elementNearBottom_ rule or possibly mark certain footer elements as such and ignore them in the bottom 15% calculation.

/cc @ampproject/wg-runtime

@aghassemi
Copy link
Contributor

My opinion is that there is no point in having a footer if user can never get to it as infinite scroll will keep pushing it down (it is actually annoying if user sees it and wants to tap on something they liked in there but by the time they try to tap it it moved away)

The footer should come as the end-element of infinite scroll.

@alabiaga
Copy link
Contributor

alabiaga commented Feb 22, 2019

Ali has a good point but to allow for more flexibility in designs, I also like the idea of making a change to allow for this. As in the example provided, there is enough delay to allow for the user to choose something from the footer and what also came to mind was b118725560; where having ad placement at the footer for monetization was required, and was preventing the auto resize from happening.

To limit the scope of the change and if the example Cathy provided is in the context of the amp-list component then perhaps we can confine this change to just that component.

@jridgewell
Copy link
Contributor

I'm fine with giving infinite scroll an exception (it may use the guaranteed changeHeight API for this one use). @choumx @dvoytenko

But I also agree with @aghassemi that infinite scrollers should never have content below them. It's just evil.

@cathyxz
Copy link
Contributor Author

cathyxz commented Feb 22, 2019

That's great feedback. So based on waht @aghassemi said about the late-arriving list content pushing down an interactable footer, I don't think we want to actually override attemptChangeHeight with changeHeight in all cases, though I think there may still be some valid situations with very long footers (i.e. disclaimer text, etc.).

I'm not sure if I can express this sufficiently clearly, but based on feedback from @andrewwatterson, I think an ideal UX which we should potentially look at supporting in this scenario looks something like this:

img_0276

  1. We have sticky footer, that shows (animates up) when the user reaches the end of the page (maybe the list is still loading).
  2. When the list finishes loading, the new items are hidden underneath this sticky footer (the footer does not move), and some UI element indicates that more items are available.
  3. When the user scrolls, the sticky footer element hides itself (animates down) and goes away until the user next reaches the end of the page.

What do people think?
(This UX actually doesn't have a whole lot to do with the elementNearBottom_ resizing constraint anymore so we can rename / close this issue accordingly.)

@jridgewell
Copy link
Contributor

I think this is a discussion to have with @ampproject/wg-ui-and-a11y. Let's leave this issue open though as that UI proposal still requires an exception to the resize rules.

@andrewwatterson
Copy link
Contributor

To defend my honor, I'd clarify that I don't think this is an ideal UX solution, just one that fits within AliExpress's specific constraints (notably having a long load time between amp-list pages and needing related search terms visible for SEO purposes).

I agree with @aghassemi's point/our existing policy against letting infinite scroll make footers that are basically inaccessible, and don't think that presenting elements (suggested searches, ads, whatever) in between amp-list pages is a pattern we want to encourage. But if you REALLY NEED to do it, the interaction @cathyxz sketched out above would be... fine.

In short, -1 to prioritizing a component like this.

@dreamofabear
Copy link

Closing this issue per above discussion.

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

No branches or pull requests

6 participants