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-live-list sort order is not configurable #5396

Closed
apaleslimghost opened this issue Oct 4, 2016 · 23 comments
Closed

amp-live-list sort order is not configurable #5396

apaleslimghost opened this issue Oct 4, 2016 · 23 comments

Comments

@apaleslimghost
Copy link

Our live blogs have a configurable sort order. However, amp-live-list only supports new messages at the top. Ideally amp-live-list should have an attribute like data-sort-order="(asc|desc)" to configure this behaviour.

@ericlindley-g
Copy link
Contributor

Agreed that this would be nice to have. I'm assuming you need a sort order configurable by the user, right? Feel free to include a link to a non-AMP implementation so we can get a good idea of the UX.

This might be reasonably straightforward with single-page amp-live-list implementations, but it gets more complicated once it needs to square with pagination, since a single AMP page does not know about all the entries of a live blog—it only knows about the items contained on that page.

For instance, way a live blog were broken up across multiple pages. In newest-first order, this isn't an issue (new updates come in, and get slotted at the top of the amp-live-list). In oldest-first order, if the user is looking at page 1 of N>1, and a new update comes in, the update button would either need to

a) flip the order of the list to newest-first order and take the user to the top of the page
b) navigate the user to the bottom of page N

Or updates could be disabled in oldest-first order

There's also a question of how to toggle the sort order in a paginated situation. There would need to be a server call of some kind to fetch the data living on another page.

A good v1 for this might be to enable it for single-page implementations, and to explore options for paginated implementations as an enhancement.

cc/ @erwinmombay

@apaleslimghost
Copy link
Author

We don't need configurable by the user; our use case is we have "Live Blogs" that load new posts at the top and "Live Q&A" which load new posts at the bottom.

@ericlindley-g
Copy link
Contributor

Got it—that simplifies things :) Do you have an example of a "Live Q&A" page?

@apaleslimghost
Copy link
Author

Not on AMP, and probably not unpaywalled, sorry 😕

@ericlindley-g
Copy link
Contributor

Just to capture notes on this request, after doing a little exploration, there are at least two changes necessary to make this work:

  • Ability to slot new items in at the bottom of the list (currently new items are assumed to be at the top of the list)
  • Ability to scroll to the bottom of the list on tapping the update button (currently scrolls to the top of the list)

Also, my sense for the live Q&A use-case is that a toggle to automatically pull in all new updates (#4524) could be as appropriate as a button

@devongovett
Copy link

Our liveblogs allow insertions to occur anywhere in the liveblog, not just at the top or bottom. Ideally, amp-live-list would look at the data-sort-time attribute on the inserted posts and insert them at the correct positions, rather than all together at the top or bottom.

@ericlindley-g
Copy link
Contributor

@devongovett Agreed — it would be good to have flexibility for inserting new items, rather than the current behavior, which inserts new items at the top, and allow changes to existing items in place.

cc/ @erwinmombay

@westonruter
Copy link
Member

For fully supporting comments in the AMP plugin for WordPress, the amp-live-list component really needs to support the sort order. The ordering of WP comments is configurable in the admin:

image

For the immediate term we'll be forcing the option to be “newer” and display a notice:

image

@ericlindley-g
Copy link
Contributor

Update:

@erwinmombay is planning to work on a fix that will support the static case of oldest-first order

Requirements:

API

  • Add new attribute for sort order

  • TODO: determine naming for attribute, options:
    -- 'data-sort-order="oldest-first | newest-first"' (this one has my vote, since it seems faster to understand the behavior)
    -- boolean: 'ascending-sort-order'
    -- 'data-sort-order="(asc|desc)"'

UX

  • Scroll down when update button tapped

  • TODO: determine where to stop
    -- Option 1: Until bottom bottom of viewport intersects with bottom of new entry (this one has my vote, but would be curious what the folks on the Bulletin team would want)
    -- Option 2: until top of viewport intersects with top of new entry (or reaches end of document)

  • New items are automatically inserted at bottom of list (instead of top)

Not needed:

  • Because the order of items on page is already determined by the developer, there's no need to adjust this

Note: With this update, amp-live-list would work for items in chronological order, but users would not yet be able to switch the ordering dynamically, as per @westonruter 's request—though @erwinmombay , would that be possible to do?

@westonruter
Copy link
Member

but users would not yet be able to switch the ordering dynamically

@ericlindley-g I should clarify that users do not need to dynamically change the ordering when viewing the list. The switching of the sort order is a setting in the admin. In other words, WordPress just needs to be able to change the ordering of the comments when first rendering the page. They don't need to change while the page has loaded. Dynamically changing the order after could be nice feature, but it would not be a core requirement.

@ericlindley-g
Copy link
Contributor

Thanks for clarifying, @westonruter — glad this will already work for WP!

@westonruter
Copy link
Member

westonruter commented Mar 17, 2018

@ericlindley-g It doesn't seem that sort=ascending is working. I've deployed the changes to WordPress commenting to https://commentsort-ampconfdemo.pantheonsite.io/2018/02/06/are-you-there-lebron-its-me-lebron-a-superstars-ultimate-pep-talk/ With the experiment enabled and sort=ascending added to the amp-live-list, new entries are still appearing at the top instead of the bottom as I recorded here: https://youtu.be/qfPzmAhON2E

@erwinmombay
Copy link
Member

@westonruter thanks for testing. Did you happen to also turn on dev-channel? (canary) i see the version is ending in "230" which probably doesn't have the latest code yet. It should be in prod this upcoming tuesday.

@westonruter
Copy link
Member

@erwinmombay Thanks. That seems to have done the trick. It seems to test I have to both enable dev-channel and amp-live-list-sorting experiments on https://cdn.ampproject.org/experiments.html and then also toggle the experiments via the AMP.toggleExperiment() calls in the console?

@erwinmombay
Copy link
Member

@westonruter just need to have both experiments on (dev-channel and amp-live-list-sorting) either through the web page or the toggling it on the console.

@erwinmombay
Copy link
Member

I should unflag the feature soon, i just need to fix the UX bug. working on it this week

@erwinmombay
Copy link
Member

The UX fix and unflagging is currently in canary. everything should be fully live by tuesday next week. I'll check up on the validator changes too and report here when i know its in

@westonruter
Copy link
Member

@erwinmombay It looks like this is live everywhere now except in the validator, is that right?

@erwinmombay
Copy link
Member

@westonruter i believe that is correct. it should get picked up on the next validator rollup

@westonruter
Copy link
Member

@erwinmombay when will the next validator rollup happen?

@erwinmombay
Copy link
Member

@westonruter i was told next week. There was some confusion if the code was reverted because master was a red a few weeks ago and this was one of those pr's that wasnt pulled in into the validator. apologize for the delay

@westonruter
Copy link
Member

I see the validator updates are now live.

@ericlindley-g
Copy link
Contributor

Closing this, but please reopen if this is not fixed yt

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

7 participants