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-list: Support dynamic resizing #10230

Closed
ericlindley-g opened this issue Jul 2, 2017 · 117 comments
Closed

amp-list: Support dynamic resizing #10230

ericlindley-g opened this issue Jul 2, 2017 · 117 comments

Comments

@ericlindley-g
Copy link
Contributor

ericlindley-g commented Jul 2, 2017

I think this is not currently possible, but would be worth double-checking

/cc @choumx — WDYT about this as a feature for amp-list + amp-bind?

EDIT: attempting to summarize the discussion below into specifications for this feature request.

Dynamic resizing needs to cover the following cases:

  1. Users interacting with the content inside the list can cause the contents to change size:
    a) Opening / closing an <amp-accordion>.
    b) Class toggles from <amp-bind>.
  2. Using amp-bind to change the src attribute of <amp-list> can cause us to fetch contents that will have a different height from <amp-list>'s current height.
  3. It is possible for <amp-list> to contain elements that will be loaded later. E.g. collapsible ads. <amp-list> should intelligently adjust its height based on the actual height of its contents.
  4. After applying filters, <amp-list> might contain fewer elements and would hence need to resize.

Caveats to note:

  1. Should not resize list when list bottom is in viewport for UX reasons.
  2. Should make sure to test <amp-list> in <amp-sidebar> use case.
  3. Should not resize if we scrolled past the <amp-list>.

Tentative proposed solution(s):

  1. Set <amp-list> to behave like layout CONTAINER after first load.
  2. Allow automatic resizing when [state] or [src] bindings change, perhaps toggled via a new boolean attribute resizeOnChange.
@aghassemi
Copy link
Contributor

are width, height, layout not bindable? seems useful regardless of use-case with amp-list.

@ericlindley-g
Copy link
Contributor Author

I believe width and height are bindable — not sure about layout, though: do you have a use case in mind for that?

@gurmukhp
Copy link

gurmukhp commented Jul 11, 2017

This would be super useful to have. We're building a product listing page (an ampStart template) and would like to update the products based on an input select change. Unfortunately, the height of the amp-list element doesn't get updated.

Simple example here
https://gurmukh-test.firebaseapp.com/templates/e-commerce/product-listing.amp

@ericlindley-g could this be prioritised?

@ericlindley-g
Copy link
Contributor Author

@aghassemi one wrinkle for this request is that amp-list would have to resize based on the data pulled in, so in some cases this wouldn't be known by the client until the data is rendered by the template

@choumx@gurmukhp is working on AMP Start templates, and the ability for amp-list to resize when src is updated by amp-bind would be useful there (product listing page with filtering capabilities which vary the number of products displayed via amp-list, using amp-bind). Is this something you've thought about before, or would be easy to fold into the other amp-list work you're looking at? @gurmukhp and team are working on templates that we're hoping to launch soon, and it would be ideal not to have to design around this issue at launch.

@dreamofabear
Copy link

Just verified that width and height bindings work in amp-list on a local example. layout currently isn't bindable.

@gurmukhp The amp-list element in that page doesn't appear to have a width/height binding.

@ericlindley-g
Copy link
Contributor Author

@choumx — does this allow amp-list to resize based on the content pulled in via JSON, as opposed to a specific height pre-determined on the client?

For instance: I give users the ability to filter by price and color of a product, but I'm not able to hard-code on the frontend the exact height for each price/color combination. What I would need would be for amp-list to dynamically resize based on the height of the items it pulls in through JSON (which would also accommodate for elements (like text) that change height based on viewport width. Is that currently supported?

@dreamofabear
Copy link

Yes, by using amp-state as an intermediary store. Just tested something like this locally:

<amp-state id="myData" [src]="'http://data.com/json?filter=' + myFilterVar">
  <script type="application/json">
    {
      "items": [] // Insert default data here.
    }
  </script>
</amp-state>

<button on="tap:AMP.setState({myFilterVar: 'red'})">Set filter to 'red'</button>

<!-- "30" is the height per item. -->
<amp-list [state]="myData.items" [height]="myData.items.length * 30"></amp-list>

Probably not immediately obvious for people new to amp-bind. Good candidate for ABE. 😄

@ericlindley-g
Copy link
Contributor Author

Nice! I'm guessing this doesn't work for variable-height items like text (which could wrap and cause new lines depending on viewport width)—is that right? Either way, this is super-useful

If this doesn't work for variable-height items: would it be possible for amp-list to understand the height of its children and resize based on that when amp-bind changes its src?

@dreamofabear
Copy link

Correct, we'd have to allow amp-bind to change layout to "container". It'd be a simpler solution than the one above. Seems like a reasonable FR.

@ericlindley-g
Copy link
Contributor Author

Awesome — changing the title of the bug to reflect the request.

@ericlindley-g ericlindley-g changed the title allow resize of amp-list on updating src via amp-bind FR: allow binding to layout attribute (supports dynamic resizing when amp-bind updates amp-list[src]) Jul 12, 2017
@gurmukhp
Copy link

This feature would be really useful in the e-commerce template for AmpStart. We have a list of products that are loaded in dynamically. The height of amp-list is dependant on the width of the device so binding using the [height] attribute isn't possible.

@dreamofabear
Copy link

Thanks for the feedback.

On second thought, adding a new element-specific action called "resize" might be better than allowing binding to layout property. For one, amp-bind doesn't do anything when the property value doesn't change, e.g. [layout] changing from "container" to "container" even if [src] changes.

<amp-list [src]="myUrlVariable" id="foo" on="change:foo.resize"></amp-list>

An even simpler way is to allow automatic resizing when [state] or [src] bindings change, perhaps toggled via a new boolean attribute:

<amp-list resizeOnChange [src]="myUrlVariable"></amp-list>

@dreamofabear dreamofabear added this to Unprioritized in Runtime Aug 10, 2017
@ericlindley-g ericlindley-g added this to Needs Triage in UI Sep 22, 2017
@dreamofabear dreamofabear changed the title FR: allow binding to layout attribute (supports dynamic resizing when amp-bind updates amp-list[src]) amp-list: Support dynamic resizing Oct 3, 2017
@dreamofabear
Copy link

Good conversation thread with @aghassemi and @dvoytenko in duped issue: #11535 (comment)

@jamesshannon
Copy link
Contributor

jamesshannon commented Oct 26, 2017

Any updates here?

This is a blocker for launching a top site. SImilar to #10230 (comment) they're an ecommerce company and the sidebar is showing categories. Categories show subcategories on click. And when those are shown the sidebar is stuck on overflow: none;

Like #11535 that was dupped 3 weeks ago the logical quick solution is to add overflow: auto, but that's not allowed.

@dreamofabear dreamofabear moved this from Unprioritized to Prioritized in Runtime Oct 27, 2017
@smokeyfro
Copy link

@cathyxz thanks for clarifying :) I tested it again in my normal chrome with dev channel and amp-list-resizable-children enabled and it seems to work now.

After playing with it for a bit it seems it's just switching the layout to container . Would it not be possible to add container layout support to amp-list, so if I know the list length needs to be dynamic, I can just set the layout to container from the start?

I want to test this with our list, but don't see any way to load the canary version of amp in the WordPress plugin, so I'll need to setup a codepen or glitch project. I'll report back once I have something up.

@cathyxz
Copy link
Contributor

cathyxz commented Dec 4, 2018

Great question! Supporting layout container was my first reaction to solving this too. Unfortunately we can't support layout container out of the box due to the potential of content jumping before user interaction. This action forces the change to layout container and any subsequent content shift to be a result of direct user interaction. It might be a good idea in the future to support layout container as an api in amp-list but force it to be fixed until user interaction.

A side note: this canary will hit production I think by EOD today or tomorrow, so you will just need to turn on the experiment (which you can do via js console) when that happens.

@smokeyfro
Copy link

Thanks for the explanation. That makes sense :)

That's great news about it being available in production so soon! I was expecting we'd have to wait a couple of weeks :P

I'll keep an eye out for the update then test it out on our staging site with the example meta tag. Will let you know how it goes.

@smokeyfro
Copy link

smokeyfro commented Dec 24, 2018

Heya @cathyxz,

I just updated our grapes and wines index using the new amp-list-resizable-children experiment with changeToLayoutContainer and it seemed to be working until I checked the list on my other laptop. I had previously enabled the experiment in my console using AMP.toggleExperiment('amp-list-resizable-children') and had also enabled it in the experiments page.

I've added the meta tag to activate the experiment, but it doesn't seem to be having any effect. Is there anything else I should be doing?

You can see our dev site (with the updated list) over here:
https://winefolly.staging.wpengine.com/grapes-wines/

@morsssss
Copy link
Contributor

morsssss commented Jan 2, 2019

Hi! Has this been released to prod yet? We're eager to try this out too :)

@aghassemi
Copy link
Contributor

Discussed offline a bit in more details. tldr a new action (and equivalent bindable attribute) to turn layout of amp-list to container upong user action (e.g. toggling between list and grid views) is live but under an experiment flag so folks can try it and provide feedback

@smokeyfro
Copy link

@aghassemi Hey Ali,

I tested this on our staging and it's working as expected. Is there a way to enable this for our users on the production site? I added the experimental meta tag, but it doesn't work unless the experiment is activated in the console (or the amp experiments page).

@aghassemi
Copy link
Contributor

@devignerforhire Thanks for trying it out. I don't mind turning this into a doc-level option experiment so you can enable it with the experimental meta tag. We can keep it a doc-in for a month and unless we hear negative feedback as folks try it, we can launch end of Jan, @cathyxz thoughts?

@cathyxz
Copy link
Contributor

cathyxz commented Jan 4, 2019

I wanted to see if it was possible to address the feedback of this approach being overly verbose, but I don't think that will impact the existence or API of this action. I'm cool with this launching by the end of Jan.

@ahmedzidan
Copy link

We are using amp-state to bind the height, but we calculate the height on the server side which is not perfect and doesn't cover all the cases, so we are waiting this feature. @cathyxz we are happy to help if you need.

Our code on production you can check it out, https://iprice.my/nike/. the filter part is using amp list.

@smokeyfro
Copy link

Thanks @aghassemi and @cathyxz. This is one of the biggest challenges we've had with amp-list, so I'm excited to start using this in production.

On a side note, I'm needing to push an update to our wines index sorting (specifically changing the rarity sorting to popularity and sorting by total acreage). All works as expected when the popularity default is initially loaded or one of the other sorting options is selected, but then has a weird spacing issue below each row when switching back to the popularity default.

Adding changeToLayoutContainer fixes the issue, but since that will only be available end of Jan, I was wondering if either of you have any other ideas on how to fix the spacing issue, so I can push the update. The view switching can hold off until end of Jan, but the current sorting doesn't take into account the acreage and as such isn't really accurate, hence the update.

Any ideas would be much appreciated.

@cathyxz
Copy link
Contributor

cathyxz commented Jan 10, 2019

Just a call out to those following on this thread. We've now had this feature in experiment for a month now in canary. The main feedback I've gotten is the verbosity (which we can work on in future iterations), but so far it seems to unblock everyone that has tried it and doesn't seem to break anyone. I'd like to go ahead and launch this in the Jan 15th release if there are no objections.
Cheers,
Cathy

@dreamofabear
Copy link

Did I miss the bikeshed party? 😅 How about containerize action and [container] binding?

@cathyxz
Copy link
Contributor

cathyxz commented Jan 11, 2019

That's exactly what I proposed, but it lost the vote.... =(

@aghassemi
Copy link
Contributor

No promises but we hope we can just support <amp-list layout=container> in V2 of amp-list (with enough restrictions to make the content shifting case very unlikely and automatically handling the case where overflow happens with some default UI)

@jaygray0919
Copy link

jaygray0919 commented Jan 11, 2019

Per previous threads, we request that the <amp-list> be generated as a <span> object, not as a <div> object.
While some have suggested that CSS can be used to keep 2+ amp-lists together in a <p>, we cannot reliably and consistently achieve that use case.
We have several cases where a single paragraph uses different amp-lists.
An example is several tool-tips used within paragraph, as seen on the Washington Post
We can say from experience that it is nearly impossible to consistently produce an amp-lightbox with data from an amp-list in a format like the WaPo.
Because of the generated <div>, the values for each unique amp-list cause a line break.
If the generated amp-list structure is packaged in a <span>, we won't have to go thru different contortions to apply unreliable keep-together CSS rules.
---
to clarify:
one amp-list per tool-tip (where tool-tip is an amp-lightbox).
we've talked with WaPo tool-tip developers; their tool-tips are re-usable objects (selectable from a library).
they "drop" a specific tool-tip into articles where sports figures are mentioned.
we also want to build re-usable tool-tips, where each tool-tip is a combo amp-lightbox + amp-list.
But re-use is currently being defeated by the code generated when using an amp-list (a non-conforming inline element <div>)

@cathyxz
Copy link
Contributor

cathyxz commented Jan 11, 2019

Hi @jaygray0919 thanks a lot for your comments, but they seem parallel to this issue. I'm aware of issues you have with trying to implement a tooltip experience via amp-lightbox and I know that we have #8366. But I'm missing a bit of context on trying to produce tooltips with <amp-list>. Do you mind tagging me on the relevant issue(s), or filing a separate issue to consolidate and help us better triage? I'd like to keep discussion on this thread specific to the issue of dynamic resizing for amp-list. =)

@jaygray0919
Copy link

Hi @cathyxz - will do. Had not seen #8366 but have used other amp-list threads to present our issue. We'll update the #8366 thread with examples and past comments by @aghassemi and @choumx. We will need a systematic tool-tip solution soon, so will use #8366 to find a solution. However, the content of an amp-lightbox for a tool-tip is variable height (i.e. the size of different amp-lists supporting tool-tips is irregular). In contrast, the content of a WaPo lightbox is a regular height. So we sit on this dynamic-sizing thread because we have that issue when building our tool-tips. For example, a tool-tip that features one national language has a different height than a tool-tip featuring 10 national languages. To repeat, the content for all amp-lightbox based tool-tips is amp-list, hence why we squat on this and related amp-list and amp-mustache threads.

@cathyxz
Copy link
Contributor

cathyxz commented Jan 14, 2019

The relevant changes from #20314 should land in canary and release candidate tomorrow and land in production by next Tuesday.

@ShivamS136
Copy link

Hi,
I am still not able to get dynamic height in amp-list in AMP Email yet... If anyone has a solution please help. I want my amp-list to change its height as per the dynamically loaded data inside it on the first load.

We can use this playground code for the sample to get how to remove the empty whitespace and make the height dynamic on the first load.

@samouri
Copy link
Member

samouri commented Dec 23, 2020

@ShivamS136: it isn't a well documented (or even fully launched) feature, and it only works within email environments for now.
I believe you need to use layout=container for it to work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Runtime
Email
UI
  
Dynamic
UI Triage
  
Done
Development

No branches or pull requests