Make table in chrome:site-engagement sortable.

Make table in chrome:site-engagement sortable.

This CL makes the site engagement table sortable when the table header
column is clicked.

It also makes the values in the table round to 2 decimal places rather
than truncating to an int.

BUG=524857
Committed: https://crrev.com/96cbae9321d95c2310c2b4981d9359ce970d83e6
Cr-Commit-Position: refs/heads/master@{#357314}

[calamity, 2015-10-27 08:02:50]

calamity@chromium.org changed reviewers:
+ tsergeant@chromium.org

[tsergeant, 2015-10-27 23:35:34]

Generally looks good. I've made a couple of suggestions to use a bit more of the
Polymer ~~~magic~~~. I'm not too sure whether they'll actually make things any
cleaner/simpler, so take a look and see what you think.

https://codereview.chromium.org/1407363010/diff/1/chrome/browser/resources/en...
File chrome/browser/resources/engagement/engagement_table.html (right):

https://codereview.chromium.org/1407363010/diff/1/chrome/browser/resources/en...
chrome/browser/resources/engagement/engagement_table.html:9: <th
id="initial-sort-column" on-tap="handleSortColumnTap"
sort-key="origin">Origin</th>
Adding/removing children here for the indicator is a little weird.

You could do something like

<span class="sort-indicator" hidden$="[[!sortKeyEquals('origin',
sortKey_)]]"></span>

on each row (sortKey_ needs to be a property).

And then define the super simple sortKeyEquals function.

To get the arrow to flip depending on sort direction, you can
`reflectToAttribute` the `sortReversed_` property, then do the rest in CSS:

.sort-indicator::before {
  content: '\u25B2';
}
:host([sort-reversed_]) .sort-indicator::before {
  content: '\u25BC';
}

https://codereview.chromium.org/1407363010/diff/1/chrome/browser/resources/en...
File chrome/browser/resources/engagement/engagement_table.js (right):

https://codereview.chromium.org/1407363010/diff/1/chrome/browser/resources/en...
chrome/browser/resources/engagement/engagement_table.js:37: ready: function() {
this.sortTable_(this.$$('#initial-sort-column')); },
Use `this.$['initial-sort-column']` for selecting by id.

https://codereview.chromium.org/1407363010/diff/1/chrome/browser/resources/en...
chrome/browser/resources/engagement/engagement_table.js:71:
getTableSortFunction_: function(sortKey, reverse) {
If you make sortKey_ and sortReverse_ into properties, then you can data bind
directly to this function:

<template ... sort="[[getTableSortFunction_(sortKey_, sortReverse_)]]">

Everything should be sorted whenever key or reverse is changed without manually
changing the sort property.

[calamity, 2015-10-28 04:26:50]

https://codereview.chromium.org/1407363010/diff/1/chrome/browser/resources/en...
File chrome/browser/resources/engagement/engagement_table.html (right):

https://codereview.chromium.org/1407363010/diff/1/chrome/browser/resources/en...
chrome/browser/resources/engagement/engagement_table.html:9: <th
id="initial-sort-column" on-tap="handleSortColumnTap"
sort-key="origin">Origin</th>
On 2015/10/27 23:35:34, tsergeant wrote:
> Adding/removing children here for the indicator is a little weird.
> 
> You could do something like
> 
> <span class="sort-indicator" hidden$="[[!sortKeyEquals('origin',
> sortKey_)]]"></span>
> 
> on each row (sortKey_ needs to be a property).
> 
> And then define the super simple sortKeyEquals function.
> 
> To get the arrow to flip depending on sort direction, you can
> `reflectToAttribute` the `sortReversed_` property, then do the rest in CSS:
> 
> .sort-indicator::before {
>   content: '\u25B2';
> }
> :host([sort-reversed_]) .sort-indicator::before {
>   content: '\u25BC';
> }

Got rid of the span entirely and just used ::after on the ths.

https://codereview.chromium.org/1407363010/diff/1/chrome/browser/resources/en...
File chrome/browser/resources/engagement/engagement_table.js (right):

https://codereview.chromium.org/1407363010/diff/1/chrome/browser/resources/en...
chrome/browser/resources/engagement/engagement_table.js:37: ready: function() {
this.sortTable_(this.$$('#initial-sort-column')); },
On 2015/10/27 23:35:34, tsergeant wrote:
> Use `this.$['initial-sort-column']` for selecting by id.

Done.

https://codereview.chromium.org/1407363010/diff/1/chrome/browser/resources/en...
chrome/browser/resources/engagement/engagement_table.js:71:
getTableSortFunction_: function(sortKey, reverse) {
On 2015/10/27 23:35:34, tsergeant wrote:
> If you make sortKey_ and sortReverse_ into properties, then you can data bind
> directly to this function:
> 
> <template ... sort="[[getTableSortFunction_(sortKey_, sortReverse_)]]">
> 
> Everything should be sorted whenever key or reverse is changed without
manually
> changing the sort property.

Done.

[tsergeant, 2015-10-28 04:57:03]

https://codereview.chromium.org/1407363010/diff/20001/chrome/browser/resource...
File chrome/browser/resources/engagement/engagement_table.html (right):

https://codereview.chromium.org/1407363010/diff/20001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.html:18: <template
id="engagement-table-items" is="dom-repeat" items="{{engagementInfo}}" as="info"
sort="[[getTableSortFunction_(sortKey_, sortReverse_)]]">
Nit: wrap to 80 chars

https://codereview.chromium.org/1407363010/diff/20001/chrome/browser/resource...
File chrome/browser/resources/engagement/engagement_table.js (right):

https://codereview.chromium.org/1407363010/diff/20001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.js:29: sortKey_: {type:
String},
As discussed, maybe set the default value here

[calamity, 2015-10-28 05:43:11]

https://codereview.chromium.org/1407363010/diff/20001/chrome/browser/resource...
File chrome/browser/resources/engagement/engagement_table.html (right):

https://codereview.chromium.org/1407363010/diff/20001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.html:18: <template
id="engagement-table-items" is="dom-repeat" items="{{engagementInfo}}" as="info"
sort="[[getTableSortFunction_(sortKey_, sortReverse_)]]">
On 2015/10/28 04:57:03, tsergeant wrote:
> Nit: wrap to 80 chars

Done.

https://codereview.chromium.org/1407363010/diff/20001/chrome/browser/resource...
File chrome/browser/resources/engagement/engagement_table.js (right):

https://codereview.chromium.org/1407363010/diff/20001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.js:29: sortKey_: {type:
String},
On 2015/10/28 04:57:03, tsergeant wrote:
> As discussed, maybe set the default value here

Done.

[tsergeant, 2015-10-28 22:28:54]

lgtm

[calamity, 2015-10-29 00:30:37]

calamity@chromium.org changed reviewers:
+ dbeam@chromium.org

[calamity, 2015-10-29 00:30:37]

+dbeam for c/b/ui/webui OWNERS.

[Dan Beam, 2015-10-29 00:50:01]

sorry, but I had to look

https://codereview.chromium.org/1407363010/diff/40001/chrome/browser/resource...
File chrome/browser/resources/engagement/engagement_table.css (right):

https://codereview.chromium.org/1407363010/diff/40001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.css:31: content: '\25B2';
any reason not to use '▲' directly?

https://codereview.chromium.org/1407363010/diff/40001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.css:34:
:host([sort-reverse_]) .sort-column::after {
nit: don't use _ in this case, IMO

https://codereview.chromium.org/1407363010/diff/40001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.css:35: content: '\25BC';
same nit, re: ▼

https://codereview.chromium.org/1407363010/diff/40001/chrome/browser/resource...
File chrome/browser/resources/engagement/engagement_table.js (right):

https://codereview.chromium.org/1407363010/diff/40001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.js:12: return
url.replace(/^[a-z]*:\/\//, '');
nit: why not use URL?  that way, if HTTP was used your code would still work :)

var u = new URL(url);
return u.host + u.pathname + u.search + u.hash;

(or something like that)

https://codereview.chromium.org/1407363010/diff/40001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.js:95: return
(stripScheme(val1) > stripScheme(val2) ? 1 : -1);
don't use extra parens, especially around return

https://codereview.chromium.org/1407363010/diff/40001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.js:100:
console.log('Unsupported sort key:' + sortKey);
should this be assertNotReached()?

https://codereview.chromium.org/1407363010/diff/40001/chrome/browser/resource...
File chrome/browser/resources/engagement/site_engagement.js (right):

https://codereview.chromium.org/1407363010/diff/40001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:23: x.score =
Number(Math.round(x.score + 'e+2') + 'e-2');
nit: * 100 / 100 seems less awkward

[calamity, 2015-10-29 02:46:01]

Thanks for the review!

https://codereview.chromium.org/1407363010/diff/40001/chrome/browser/resource...
File chrome/browser/resources/engagement/engagement_table.css (right):

https://codereview.chromium.org/1407363010/diff/40001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.css:31: content: '\25B2';
On 2015/10/29 00:50:00, Dan Beam wrote:
> any reason not to use '▲' directly?

Done.

https://codereview.chromium.org/1407363010/diff/40001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.css:34:
:host([sort-reverse_]) .sort-column::after {
On 2015/10/29 00:50:00, Dan Beam wrote:
> nit: don't use _ in this case, IMO

Done.

https://codereview.chromium.org/1407363010/diff/40001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.css:35: content: '\25BC';
On 2015/10/29 00:50:00, Dan Beam wrote:
> same nit, re: ▼

Done.

https://codereview.chromium.org/1407363010/diff/40001/chrome/browser/resource...
File chrome/browser/resources/engagement/engagement_table.js (right):

https://codereview.chromium.org/1407363010/diff/40001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.js:12: return
url.replace(/^[a-z]*:\/\//, '');
On 2015/10/29 00:50:00, Dan Beam wrote:
> nit: why not use URL?  that way, if HTTP was used your code would still work
:)
> 
> var u = new URL(url);
> return u.host + u.pathname + u.search + u.hash;
> 
> (or something like that)

Oh cool. These are only ever origins anyway, so I can just take the host of
this.

https://codereview.chromium.org/1407363010/diff/40001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.js:95: return
(stripScheme(val1) > stripScheme(val2) ? 1 : -1);
On 2015/10/29 00:50:00, Dan Beam wrote:
> don't use extra parens, especially around return

Done.

https://codereview.chromium.org/1407363010/diff/40001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.js:100:
console.log('Unsupported sort key:' + sortKey);
On 2015/10/29 00:50:00, Dan Beam wrote:
> should this be assertNotReached()?

Done.

https://codereview.chromium.org/1407363010/diff/40001/chrome/browser/resource...
File chrome/browser/resources/engagement/site_engagement.js (right):

https://codereview.chromium.org/1407363010/diff/40001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:23: x.score =
Number(Math.round(x.score + 'e+2') + 'e-2');
On 2015/10/29 00:50:00, Dan Beam wrote:
> nit: * 100 / 100 seems less awkward

Done.

[Dan Beam, 2015-10-29 22:03:16]

lgtm

https://codereview.chromium.org/1407363010/diff/60001/chrome/browser/resource...
File chrome/browser/resources/engagement/engagement_table.js (right):

https://codereview.chromium.org/1407363010/diff/60001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.js:10: */
@return

https://codereview.chromium.org/1407363010/diff/60001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.js:87: * @param {string}
sortKey The name of the property to sort by.
@return

[calamity, 2015-10-30 03:56:27]

https://codereview.chromium.org/1407363010/diff/60001/chrome/browser/resource...
File chrome/browser/resources/engagement/engagement_table.js (right):

https://codereview.chromium.org/1407363010/diff/60001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.js:10: */
On 2015/10/29 22:03:16, Dan Beam wrote:
> @return

Done.

https://codereview.chromium.org/1407363010/diff/60001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.js:87: * @param {string}
sortKey The name of the property to sort by.
On 2015/10/29 22:03:16, Dan Beam wrote:
> @return

Done.

[calamity, 2015-10-30 05:06:03]

The CQ bit was checked by calamity@chromium.org

[calamity, 2015-10-30 05:06:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
tsergeant@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1407363010/#ps80001
(title: "add @returns")

[commit-bot: I haz the power, 2015-10-30 05:06:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1407363010/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1407363010/80001

[commit-bot: I haz the power, 2015-10-30 05:13:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-30 05:13:48]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[calamity, 2015-11-02 06:22:19]

The CQ bit was checked by calamity@chromium.org

[calamity, 2015-11-02 06:22:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
tsergeant@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1407363010/#ps100001
(title: "go back to normal functions to please the almighty PRESUBMIT")

[commit-bot: I haz the power, 2015-11-02 06:22:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1407363010/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1407363010/100001

[commit-bot: I haz the power, 2015-11-02 06:26:30]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-02 06:27:29]

Patchset 6 (id:??) landed as
https://crrev.com/96cbae9321d95c2310c2b4981d9359ce970d83e6
Cr-Commit-Position: refs/heads/master@{#357314}