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

Intent to implement: sortable tables #6057

Closed
DerekNonGeneric opened this issue Nov 7, 2016 · 19 comments
Closed

Intent to implement: sortable tables #6057

DerekNonGeneric opened this issue Nov 7, 2016 · 19 comments
Assignees
Labels
INTENT TO IMPLEMENT Proposes implementation of a significant new feature. https://bit.ly/amp-contribute-code Type: Feature Request

Comments

@DerekNonGeneric
Copy link

Proposal

I intend on creating a new component: <amp-sortable-table>, which will function as a standard <table> with extended functionality to allow its rows to be sorted by interacting with arrow icons in the table header. This behavior is commonly used in Wikipedia and is documented here.

Implementation

Wikipedia appears to be using the tablesorter jQuery plugin to achieve this. I have used this plugin in the past—it works well. Unfortunately, due to its dependence on jQuery, it is automatically not feasible for our purposes, so a vanilla version will have to be written.

Moving forward

If given the go-ahead, I could begin re-writing this plugin so that it does not depend on jQuery. Having this functionality is crucial to the success of my project and I believe the community would benefit greatly from having sortable tables.

I'm curious what you all think. Would you all consider adding a component like this if it was well-written and performant? Thank you for your consideration.

@DerekNonGeneric DerekNonGeneric added this to the Pending Triage milestone Nov 7, 2016
@DerekNonGeneric DerekNonGeneric added the INTENT TO IMPLEMENT Proposes implementation of a significant new feature. https://bit.ly/amp-contribute-code label Nov 7, 2016
@aghassemi
Copy link
Contributor

/to @ericlindley-g

@derekcecillewis Thanks for contributing. Let's start with a JS fiddle as proof of concept.

A basic table-sorter with limited data type detection and minimal (but extensible) built-in UI would be a nice component to have IMO.

Getting it be performant and memory efficient might take some fine tuning (e.g. is it better to pre-scan the table and keep a parallel data structure with types already detected for all cell values or is it better to do that on demand at sort time?). Efficiently redrawing the table upon sort also needs careful consideration, we want to optimize to get a single relayout of the table as a whole after sort since table layouts are expensive. (as opposed mutating each cell individually and triggering a relayout for instance). And of course there is the API design and the UX.

I think starting with a fiddle to experiment with the API, UX, data-type-detection and the redrawing approach, and collaborating on that before creating a component would be a good start.

@ericlindley-g Do we have any plans for a full-pledge Data Grid component for AMP? In particular, there is a gap in fully responsive data grids out there. Of course, data grids are really big components and can be a lot of work to provide a full-featured, performant grid implementation. ( I have prototyped the concept of responsive grids in the past - i.e. grids that collapse their columns based on priorities given by the author - but my implementation is no where near the performance expectations of AMP :) )

@camelburrito camelburrito self-assigned this Nov 8, 2016
@camelburrito
Copy link
Contributor

@derekcecillewis , before you start writing way too much code and the send it for review, i would recommend doing this in incremental steps.

Your first step would be, proposing a features and a format, which you could put in a collaborative document with public comment access. I can take it to a format review with the internal team and walk through your proposal and we can provide feedback on your proposal.

This would make things easier for you and make goals/expectations for the component more concrete.

@DerekNonGeneric
Copy link
Author

Here is the collaborative document with public comment access: AMP Sortable Tables.

@DerekNonGeneric
Copy link
Author

DerekNonGeneric commented Feb 6, 2017

Here is the JSFiddle proof of concept: AMP Sortable Tables Prototype.

@camelburrito
Copy link
Contributor

@derekcecillewis - hope you saw my comments on slack - i created a sortable table prototype with flexboxes - check it out :)

http://output.jsbin.com/webuval

And you can use it to compare performance.

@camelburrito
Copy link
Contributor

Table + Refilling DOM

screen shot 2017-02-07 at 12 52 34 am

Flexbox Performance
screen shot 2017-02-07 at 12 51 20 am

Looks like Flexbox ordering is a lot more performant. (and this can still be optimized)

@DerekNonGeneric
Copy link
Author

DerekNonGeneric commented Feb 8, 2017

Wow, I was not aware of the Flexbox order property. I'm surprised this was not mentioned when we were drafting the design doc. Great idea! Although I hypothesize you are correct in that Flexbox ordering is more performant, quite a bit of your prototype's logic is very different from mine. Unless you feel the ~10ms time difference is large enough to come to a conclusion, maybe a proper experiment is in order?

Edit: As you mentioned to me on Slack, you were speaking strictly about the rendering performance and that these numbers would remain the same regardless of the scripting logic. That being said, it seems as if a proper experiment testing rendering performance may be unnecessary.

@aghassemi
Copy link
Contributor

@derekcecillewis @camelburrito Flexbox ordering is a brilliant solution! I am worried however that TalkBack and VoiceOver may not yet support order yet. My recommendation is testing this with those screen readers, if they pick up proper ordering, let's do order, if not, we need to stick with repositioning the elements for now.

@camelburrito
Copy link
Contributor

I don't think holding all the table's data in memory is a great idea. Flexbox solution is super performant, but if we dont wanna lose accessibility just for the sort (the table itself in general is accessible) then i would say may be DOM re-ordering is better. We could check if react/polymer do something similar.

Also we have some precedent using flexbox ordering for loops in Slides (might not be great for a11y)

@dvoytenko @cramforce - WDYT

@DerekNonGeneric
Copy link
Author

DerekNonGeneric commented Feb 19, 2017

Good point @aghassemi. I've tested VoiceOver on MacOS Sierra Safari and it does not seem to respect Flexbox ordering. It continues to read the table in the order that it appears in the HTML after being sorted. As I do not have an Android device, I could not test TalkBack. According to my research, there is no ARIA support for Flexbox ordering at the moment. The only workaround I was able to find is to use aria-flowto, but that does not appear to be supported by VoiceOver. You can use my reduction for testing TalkBack on Android if you want.

@ericlindley-g ericlindley-g modified the milestones: Pending 3P Implementation, Pending Triage Feb 21, 2017
@aghassemi
Copy link
Contributor

@derekcecillewis Thanks for evaluating the approaches for a11y. I am not surprised that VoiceOver and TalkBack don't support flexbox ordering or aria-flowto (unfortunate, but assertive technologies are normally one step behind browsers in feature support). Considering sort not working for screen readers is a deal breaker for a11y, I recommends moving ahead with your current approach of reordering the DOM nodes. @camelburrito WDYT?

@DerekNonGeneric
Copy link
Author

DerekNonGeneric commented Feb 27, 2017

@aghassemi, my initial solution stores all of the data in a 2D array, sorts that 2D array, generates a new DOM collection based on the sorted 2D array, and then "refills" the table's parent node with the generated one. This is what we agreed on in the design doc. @camelburrito says that he doesn't want to store all of the table's data in memory. I am under the impression that this to guard against potential memory restrictions. When we were drafting the design doc, I was told that when doing what I understand to be reparenting, it would cause too many repaints. As requested, I have reimplemented the prototype to perform reparenting: AMP Sortable Table Prototype (ignore the colors), which reorders the DOM nodes. Before I continue with providing full functionality, can you please confirm that this is the agreed upon logic? Also, please check out the ARIA tags I have added and confirm that this is what was expected. One more thing: can we shorten the name of the attribute that indicates sort direction (sorted) to simply sort (similar to the ARIA equivalent) and change the name of attribute not-sortable to simply unsortable? Thanks in advance! If everything checks out, I'll make the initial PR right away.

@aghassemi
Copy link
Contributor

@derekcecillewis 👍 for both reparenting approach and attribute name changes.

@ericlindley-g
Copy link
Contributor

/cc @choumx

@DerekNonGeneric
Copy link
Author

I apologize for my lack of input the past couple of weeks. I intended to move forward on this the week of 3/13 as I stated in the weekly status, but unfortunately had to tend to higher priority tasks. I have been hesitant to continue moving forward as the following week @ericlindley-g proposed support for filterable tables in a separate issue (#8368) and suggested we combine sorting and filtering into a single amp-table component. I think it's potentially a good idea, but definitely requires further discussion. I'd like to request that the brainstorming intended for Tuesday occur in public on Wednesday during the design review (#8193). Ordinarily I am preoccupied during this time, but am willing to shift things around in my schedule in order to attend. @aghassemi @camelburrito @choumx @ericlindley-g do you think we can make this happen? I'd be disappointed to be left out of the loop on something which concerns a component I have put so much effort into. Thank you for your consideration.

@ericlindley-g
Copy link
Contributor

Hey Derek — no worries on having to take a break to work on higher priority things. The brainstorm tomorrow is an exploratory half-day research and hacking session: too long and too unfocused for design review (will mostly be heads down time). The exploration is in part to see if it even makes sense to do things as I initially proposed, or if there's a better solution. After thinking through this more with other folks, it sounds less and less likely that there will be a single combined amp-table component (so it may not affect the sortable tables work directly), but more discovery and experimentation is needed.

Either way, the plan is to connect with you after tomorrow's research to figure out the best way to move forward—and definitely * not * to leave you out of the loop. What do you think? (Feel free to ping on Slack to talk through these logistics, as well)

@DerekNonGeneric
Copy link
Author

I'd be interested to know the result of the brainstorming around filterable tables and whether this component will be implicated. I'll reach out to you on Slack later today to catch up on the details. Thanks for the speedy response and I hope today's research and hacking goes well.

@aghassemi
Copy link
Contributor

@derekcecillewis We ended up focusing on server-side sorting, filtering and pagination which requires a fairly different approach and uses amp-list. @choumx will be leading that.

We briefly chatted about client-side sort and filter as well. We have some ideas for that. In short, I am hoping we can provide a more generic client-side sorting solution that works for tables, lists or even bunch of random divs. Most of the concepts from sortable-table applies directly to the new generic component anyway.

I am planning to spend the rest of the day to do a proof of concept for <amp-sort> and <amp-filter> and write a one-pager design doc.

If you still have cycles and interest in contributing, you should be able to adopt your existing solution for table to work for the generic <amp-sort> as well. Depending on your interest, ` is also something you can take on if you like!

I will ping this thread when I have the proof of concept and design ready.

@aghassemi
Copy link
Contributor

Closing in favour of #8691

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
INTENT TO IMPLEMENT Proposes implementation of a significant new feature. https://bit.ly/amp-contribute-code Type: Feature Request
Projects
None yet
Development

No branches or pull requests

6 participants