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

Intent to Implement: amp-sort #8691

Open
aghassemi opened this Issue Apr 11, 2017 · 17 comments

Comments

@aghassemi
Member

aghassemi commented Apr 11, 2017

Background

sortable tables - #6057 outlines a design by @derekcecillewis for supporting sortable tables in AMP.

amp-sort is a proposal to bundle most of the concepts from the sortable tables into a generic client-side sorting component that can support tables as well as lists and other arbitrary DOMs.

Proposal

amp-sort is a functional AMP extension that can rearrange a subset of its descendants by comparing values of HTML attributes which are present on the descendants.

Simple Example

<button on="tap:priceSorter.sort();">Sort by price - High to Low</button>
<amp-sort id="priceSorter" layout="container"
          sort-by="data-price" 
          sort-direction="desc" 
          value-type="number" >
  <ul>
    <li data-price="30">Red Shirt - $30</li>
    <li data-price="10">Blue T-Shirt - $10</li>
    <li data-price="20">Green Hoodie - $20</li>
  </ul>
</amp-sort>

Demos

Following page has 3 working demos of amp-sort
https://amp-sort.firebaseapp.com/test/manual/amp-sort.amp.html

sort-listgif

ezgif-3-f534e5c438

Behaviour

  • amp-sort can contain any other HTML or AMP element (including nested amp-sort)
  • amp-sort performs the following actions when .sort() is called:
    1. Finds all the descendant elements that have the attribute specified by sort-by
    2. Extracts the value of the attribute for each element and casts to the type specified by value-type
    3. Sorts the elements using the now-typed value in ascending or descending order based on sort-direction
    4. Rearranges the elements in the DOM based on the new sort order.
    5. Adds sorted, sorted-by=<sort-by value>, sorted-by=<sort-direction value> to the amp-sort component so CSS can be used to target the sorted items (e.g. hide/show sorted UI indicators, etc...)
  • amp-sort works with and without amp-bind.

Detailed API

Attributes

sort-by

The name of the attribute on descendants which its value is used for sorting comparison.

sort-by is bindable via amp-bind and triggers a resort when changed.

sort-direction

One of asc, desc, toggle. Defaults to asc.

asc or desc specify if sorting should be in ascending or descending order.
toggle will reverse the previously used sort direction.

sort-direction is bindable via amp-bind and triggers a resort when changed.

value-type

One of string, number. Defaults to string.

Specifies the data-type of the values of the attribute specified by sort-by.

value-type is bindable via amp-bind and triggers a resort when changed.

Actions

sort([sortBy:<attr>], [sortDirection:<dir>], [valueType=<type>])

Calling sort() action on amp-sort will trigger sorting. Optionally new sorting configuration can be provided to override existing configuration attributes.

Complex Examples

Multiple Sort Fields

without amp-bind

<button on="tap:sorter.sort(sortBy='data-price', sortDirecion='desc')">PRICE: high to low</button>
<button on="tap:sorter.sort(sortBy='data-price', sortDirecion='asc')">PRICE: low to high</button>
<button on="tap:sorter.sort(sortBy='data-is-new', sortDirecion='desc')">DATE: newest first</button>

<amp-sort id="sorter" value-type="number">
  <ul class="items">
    <li class="card" data-price="20" data-is-new="1">
      <amp-img layout="fill" src="https://placeholdit.imgix.net/~text?&w=400&h=400&txtsize=70&bg=d50000&txt=$20 - New!">
      </amp-img>
    </li>

    <li class="card" data-price="10" data-is-new="0">
      <amp-img layout="fill" src="https://placeholdit.imgix.net/~text?&w=400&h=400&txtsize=70&bg=4CAF50&txt=$10">
      </amp-img>
    </li>
    ...
  </ul>
<amp-sort>

with amp-bind

<button on="tap:AMP.setState({sortBy: 'data-price'}), AMP.setState({sortDir: 'desc'})">PRICE: high to low</button>
<button on="tap:AMP.setState({sortBy: 'data-price'}), AMP.setState({sortDir: 'asc'})">PRICE: low to high</button>
<button on="tap:AMP.setState({sortBy: 'data-is-new'}), AMP.setState({sortDir: 'desc'})">DATE: newest first</button>

<amp-sort [sort-by]="sortBy" [sort-direction]="sortDir" value-type="number">
  <ul class="items">
    <li class="card" data-price="20" data-is-new="1">
      <amp-img layout="fill" src="https://placeholdit.imgix.net/~text?&w=400&h=400&txtsize=70&bg=d50000&txt=$20 - New!">
      </amp-img>
    </li>

    <li class="card" data-price="10" data-is-new="0">
      <amp-img layout="fill" src="https://placeholdit.imgix.net/~text?&w=400&h=400&txtsize=70&bg=4CAF50&txt=$10">
      </amp-img>
    </li>
    ...
  </ul>
<amp-sort>

Sorting Tables

Sorting tables is not fundamentally different than the examples above however the UX
of sortable tables normally involves having column headers toggle sort direction and there is
usually visible sort indicators on sorted columns.

ezgif-3-f534e5c438

HTML

<amp-sort id="tableSorter" value-type='number'>
  <table summary="Table shwowing items, their price and whether they are new">
    <thead>
      <th>Item</th>
      <th on="tap:tableSorter.sort(sortBy='data-price', sortDirecion='toggle')" class="priceCol">Price</th>
      <th on="tap:tableSorter.sort(sortBy='data-is-new', sortDirecion='toggle')" class="isNewCol">Is New</th>
    </thead>
    <tbody>
      <tr data-price="20" data-is-new="1">
        <th scope="row">Red Jacket</th>
        <td>$20</td>
        <td>Yes</td>
      </tr>
      <tr data-price="10" data-is-new="0">
        <th scope="row">Green Shoes</th>
        <td>$10</td>
        <td>No</td>
      </tr>
      ...
    </tbody>
  </table>
<amp-sort>

CSS

As mentioned in the Behaviour section, amp-sort adds sorted, sorted-by=<sort-by value>, sorted-by=<sort-direction value> to the amp-sort component so CSS can be used to target the sorted items. We will take advantages of that here:

.priceCol, .isNewCol {
    cursor: pointer;
  }

.priceCol::after, .isNewCol::after {
  content: ' ⇅';
  color: #AAA;
}

amp-sort[sorted-by='data-price'][sorted-direction='asc'] .priceCol::after,
amp-sort[sorted-by='data-is-new'][sorted-direction='asc'] .isNewCol::after {
  content: ' ⇣';
  color: deeppink;
}

amp-sort[sorted-by='data-is-new'][sorted-direction='desc'] .isNewCol::after,
amp-sort[sorted-by='data-price'][sorted-direction='desc'] .priceCol::after {
  content: ' ⇡';
  color: deeppink;
}

A11Y Considerations

  • amp-sort actually moves the DOM elements upon sorting so DOM order will correctly represent thevisual order. (As opposed to using CSS order property which does not reorder the actual DOM and is not supported by almost all screen readers).
  • amp-sort detects tables and additionally adds aria-sort property on the table header.

Performance Considetations

  • When changing multiple configurations (e.g. both sort-by and sort-direction), actual sorting only happens once and not be config change. This is because:
    1. amp-bind already handles batching and only triggers a mutation once when multiple states are change synchronously together.
    2. sort() action optionally takes all the configuration so multiple config changes can be bundled with a single call to sort()
  • amp-sort to ensure only a single browser relayout/paint to happen after all DOM elements have been moved. This should already be the case due to AMP's builtin measure/mutate cycles ensuring no read is happening while the mutation cycle is rearranging the DOM elements.

Prototype Reference Code

/cc @dvoytenko @ericlindley-g @derekcecillewis

@ericlindley-g

This comment has been minimized.

Show comment
Hide comment
@ericlindley-g

ericlindley-g Apr 11, 2017

Collaborator

Nice—excited to see this handling a broad case

Looks like filtering would be fairly straightforward to add on to this, using a similar model. Is that a fair assumption? That is, would amp-filter (or however it might end up getting implemented) be able to take advantage of the same structure of data-* attributes to filter on, or would it need a different approach?

Collaborator

ericlindley-g commented Apr 11, 2017

Nice—excited to see this handling a broad case

Looks like filtering would be fairly straightforward to add on to this, using a similar model. Is that a fair assumption? That is, would amp-filter (or however it might end up getting implemented) be able to take advantage of the same structure of data-* attributes to filter on, or would it need a different approach?

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi Apr 11, 2017

Member

@ericlindley-g amp-filter is conceptually fairly similar (but with a bigger API surface). I am mostly done with amp-filter design but haven't had time to write the design doc yet (will be next week to present in the Format Review). You can take an early look at what I have in mind for filter here: https://github.com/aghassemi/amphtml/blob/filtersort-proto1/test/manual/amp-filter-sort.amp.html

Member

aghassemi commented Apr 11, 2017

@ericlindley-g amp-filter is conceptually fairly similar (but with a bigger API surface). I am mostly done with amp-filter design but haven't had time to write the design doc yet (will be next week to present in the Format Review). You can take an early look at what I have in mind for filter here: https://github.com/aghassemi/amphtml/blob/filtersort-proto1/test/manual/amp-filter-sort.amp.html

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi
Member

aghassemi commented Apr 11, 2017

@dvoytenko

This comment has been minimized.

Show comment
Hide comment
@dvoytenko

dvoytenko Apr 12, 2017

Collaborator

@aghassemi This looks great. Couple notes/questions:

Attribute: sort-by

We'd need to document clearly that we do not do initial reshuffling, right? In other words, if it says sort-by="date", we expect the DOM already be presorted by date at the page gen time. Right?

toggle will reverse the previously used sort direction.

The default value is often sensitive to value type. E.g. strings default to "asc" and numbers "desc". Maybe we can come up with some coding for this to leave it to the page author? E.g. sort(direction='toggle|desc')?

sorter.sort(...)

I assume, we'd never need something like resetSort() or thenSort to compound sortings. Right?

Collaborator

dvoytenko commented Apr 12, 2017

@aghassemi This looks great. Couple notes/questions:

Attribute: sort-by

We'd need to document clearly that we do not do initial reshuffling, right? In other words, if it says sort-by="date", we expect the DOM already be presorted by date at the page gen time. Right?

toggle will reverse the previously used sort direction.

The default value is often sensitive to value type. E.g. strings default to "asc" and numbers "desc". Maybe we can come up with some coding for this to leave it to the page author? E.g. sort(direction='toggle|desc')?

sorter.sort(...)

I assume, we'd never need something like resetSort() or thenSort to compound sortings. Right?

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi Apr 12, 2017

Member

We'd need to document clearly that we do not do initial reshuffling, right? In other words, if it says sort-by="date", we expect the DOM already be presorted by date at the page gen time. Right?

Yes, no initial sorting however there is no requirement that initial order matches sort-by.

The default value is often sensitive to value type. E.g. strings default to "asc" and numbers "desc". Maybe we can come up with some coding for this to leave it to the page author? E.g.

Good point. I like the format you have, reads nicely too "toggle or descending"

I assume, we'd never need something like resetSort() or thenSort to compound sortings. Right?
I don't think we need reset(), reset can be done as just another amp-sort with initial sorting criteria.

We probably need to support compound sorting to allow breaking ties in meaningful ways maybe: sort-by="data-price, data-is-new" sort-direction="asc, desc" so same price items are secondarily sorted by date.

Member

aghassemi commented Apr 12, 2017

We'd need to document clearly that we do not do initial reshuffling, right? In other words, if it says sort-by="date", we expect the DOM already be presorted by date at the page gen time. Right?

Yes, no initial sorting however there is no requirement that initial order matches sort-by.

The default value is often sensitive to value type. E.g. strings default to "asc" and numbers "desc". Maybe we can come up with some coding for this to leave it to the page author? E.g.

Good point. I like the format you have, reads nicely too "toggle or descending"

I assume, we'd never need something like resetSort() or thenSort to compound sortings. Right?
I don't think we need reset(), reset can be done as just another amp-sort with initial sorting criteria.

We probably need to support compound sorting to allow breaking ties in meaningful ways maybe: sort-by="data-price, data-is-new" sort-direction="asc, desc" so same price items are secondarily sorted by date.

@ericlindley-g ericlindley-g added this to Needs Triage in UI Apr 14, 2017

@ericlindley-g ericlindley-g moved this from Needs Triage to Backlog in UI May 25, 2017

@ericlindley-g ericlindley-g moved this from Backlog to Backlog (shortlist) in UI May 25, 2017

@dereklewis

This comment has been minimized.

Show comment
Hide comment
@dereklewis

dereklewis Jun 16, 2017

As it is developed, the documentation page is able to be previewed here: https://dereklewis.github.io/amp-docs/docs/reference/components/amp-sort/

Anticipated completion: prior to the Q2 mid-quarter roadmap update.

dereklewis commented Jun 16, 2017

As it is developed, the documentation page is able to be previewed here: https://dereklewis.github.io/amp-docs/docs/reference/components/amp-sort/

Anticipated completion: prior to the Q2 mid-quarter roadmap update.

@ampprojectbot

This comment has been minimized.

Show comment
Hide comment
@ampprojectbot

ampprojectbot Oct 30, 2017

Collaborator

This issue hasn't been updated in awhile. @aghassemi Do you have any updates?

Collaborator

ampprojectbot commented Oct 30, 2017

This issue hasn't been updated in awhile. @aghassemi Do you have any updates?

@dereklewis

This comment has been minimized.

Show comment
Hide comment
@dereklewis

dereklewis Oct 30, 2017

The rest of the code is on my drive in my apartment out of state. I will be returning in December, at which point I will be making the PR. My apologies for the delay; thanks for your patience!

dereklewis commented Oct 30, 2017

The rest of the code is on my drive in my apartment out of state. I will be returning in December, at which point I will be making the PR. My apologies for the delay; thanks for your patience!

@grantkemp

This comment has been minimized.

Show comment
Hide comment
@grantkemp

grantkemp Nov 16, 2017

Hi we are working on a couple of retail sites - this is a blocker for us. Currently we are doing a lot of engineering server side- would love this feature- as it means we can launch a lot quicker.

grantkemp commented Nov 16, 2017

Hi we are working on a couple of retail sites - this is a blocker for us. Currently we are doing a lot of engineering server side- would love this feature- as it means we can launch a lot quicker.

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi Nov 16, 2017

Member

/cc @lswang1618

@dereklewis we need to prioritize this for e-commerce. Would you be able to send a PR with your work in progress?

Member

aghassemi commented Nov 16, 2017

/cc @lswang1618

@dereklewis we need to prioritize this for e-commerce. Would you be able to send a PR with your work in progress?

@dereklewis

This comment has been minimized.

Show comment
Hide comment
@dereklewis

dereklewis Nov 16, 2017

Yes, I'll get that WIP PR in today. My apologies for the delay, unfortunately life showed up for me.

dereklewis commented Nov 16, 2017

Yes, I'll get that WIP PR in today. My apologies for the delay, unfortunately life showed up for me.

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi Nov 16, 2017

Member

back to P2 after chatting with @lswang1618

Member

aghassemi commented Nov 16, 2017

back to P2 after chatting with @lswang1618

@ericlindley-g

This comment has been minimized.

Show comment
Hide comment
@ericlindley-g

ericlindley-g Nov 17, 2017

Collaborator

FYI got several requests for this and amp-filter at the AMP roadshow today, and a few folks who are also interested in helping out that may jump in on the thread.

Collaborator

ericlindley-g commented Nov 17, 2017

FYI got several requests for this and amp-filter at the AMP roadshow today, and a few folks who are also interested in helping out that may jump in on the thread.

@dereklewis

This comment has been minimized.

Show comment
Hide comment
@dereklewis

dereklewis Nov 20, 2017

Birthday celebrations prevented me from getting to this as I would have liked. @ericlindley-g, it would be great if you could find someone to take care of the unit tests for these components (the more the merrier). Additionally, I am currently in a situation where I have a conflict of interest in making contributions to Google open-source code bases. I give @aghassemi permission to be the sole owner of this component. Feel free to copy and paste all of the code verbatim, no attribution necessary.

dereklewis commented Nov 20, 2017

Birthday celebrations prevented me from getting to this as I would have liked. @ericlindley-g, it would be great if you could find someone to take care of the unit tests for these components (the more the merrier). Additionally, I am currently in a situation where I have a conflict of interest in making contributions to Google open-source code bases. I give @aghassemi permission to be the sole owner of this component. Feel free to copy and paste all of the code verbatim, no attribution necessary.

@grantkemp

This comment has been minimized.

Show comment
Hide comment
@grantkemp

grantkemp Nov 20, 2017

Great work on what you are doing all !
Hi all - after the London roadshow, a few of us users are getting together to help advocate use of features with clients and also to help on some needed features. - ideally as part of projects. This I think is a good example to help

If you can clearly let us know what things you looking for help with ( both beginners tasks, intermediate and advanced then I am sure there are users that would help share the load)

grantkemp commented Nov 20, 2017

Great work on what you are doing all !
Hi all - after the London roadshow, a few of us users are getting together to help advocate use of features with clients and also to help on some needed features. - ideally as part of projects. This I think is a good example to help

If you can clearly let us know what things you looking for help with ( both beginners tasks, intermediate and advanced then I am sure there are users that would help share the load)

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi Nov 20, 2017

Member

@dereklewis thanks for your work on this. Given you are no longer working on the component, we will start from scratch to turn my prototype (https://github.com/aghassemi/amphtml/blob/filtersort-proto1/extensions/amp-sort/0.1/amp-sort.js) into a component.

Member

aghassemi commented Nov 20, 2017

@dereklewis thanks for your work on this. Given you are no longer working on the component, we will start from scratch to turn my prototype (https://github.com/aghassemi/amphtml/blob/filtersort-proto1/extensions/amp-sort/0.1/amp-sort.js) into a component.

@ericlindley-g ericlindley-g moved this from Backlog (shortlist) to E-comm in UI Jan 5, 2018

@ampprojectbot

This comment has been minimized.

Show comment
Hide comment
@ampprojectbot

ampprojectbot Feb 26, 2018

Collaborator

This issue hasn't been updated in awhile. @aghassemi Do you have any updates?

Collaborator

ampprojectbot commented Feb 26, 2018

This issue hasn't been updated in awhile. @aghassemi Do you have any updates?

@aghassemi aghassemi modified the milestones: Prioritized FRs, New FRs Feb 26, 2018

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