From 04fc3c0483cd937d26c303f407cf7bc6b04c7e30 Mon Sep 17 00:00:00 2001
From: yan <yan@mit.edu>
Date: Thu, 15 Sep 2016 17:52:37 -0700
Subject: [PATCH] refactor SortableTable to explicitly take HTML and sort value

fix #4047
minor UI improvements
---
 js/about/history.js            | 11 ++++--
 js/about/preferences.js        | 19 ++++++++--
 js/components/sortableTable.js | 68 ++++++++++++++++------------------
 less/sortableTable.less        |  6 +++
 4 files changed, 60 insertions(+), 44 deletions(-)

diff --git a/js/about/history.js b/js/about/history.js
index 67b70c6ad77..64489649c9e 100644
--- a/js/about/history.js
+++ b/js/about/history.js
@@ -34,10 +34,15 @@ class HistoryDay extends ImmutableComponent {
     return <div>
       <div className='sectionTitle historyDayName'>{this.props.date}</div>
       <SortableTable headings={['time', 'title', 'domain']}
+        defaultHeading='time'
+        defaultHeadingSortOrder='desc'
         rows={this.props.entries.map((entry) => [
-          entry.get('lastAccessedTime')
-            ? new Date(entry.get('lastAccessedTime')).toLocaleTimeString()
-            : '',
+          {
+            html: entry.get('lastAccessedTime')
+              ? new Date(entry.get('lastAccessedTime')).toLocaleTimeString()
+              : '',
+            value: entry.get('lastAccessedTime')
+          },
           entry.get('customTitle') || entry.get('title')
             ? entry.get('customTitle') || entry.get('title')
             : entry.get('location'),
diff --git a/js/about/preferences.js b/js/about/preferences.js
index d2180d7a7e8..e6bc432b593 100644
--- a/js/about/preferences.js
+++ b/js/about/preferences.js
@@ -600,10 +600,20 @@ class SearchTab extends ImmutableComponent {
         display: 'inline-block',
         verticalAlign: 'middle'
       }
-      array.push([<SearchSelectEntry name={entry.name} settings={this.props.settings} />,
-        <SearchEntry name={entry.name} iconStyle={iconStyle}
-          onChangeSetting={this.props.onChangeSetting} />,
-        <SearchShortcutEntry shortcut={entry.shortcut} />])
+      array.push([
+        {
+          html: <SearchSelectEntry name={entry.name} settings={this.props.settings} />,
+          value: entry.name
+        },
+        {
+          html: <SearchEntry name={entry.name} iconStyle={iconStyle} onChangeSetting={this.props.onChangeSetting} />,
+          value: entry.name
+        },
+        {
+          html: <SearchShortcutEntry shortcut={entry.shortcut} />,
+          value: entry.shortcut
+        }
+      ])
     })
     return array
   }
@@ -616,6 +626,7 @@ class SearchTab extends ImmutableComponent {
     return <div>
       <div className='sectionTitle' data-l10n-id='searchSettings' />
       <SortableTable headings={['default', 'searchEngine', 'engineGoKey']} rows={this.searchProviders}
+        defaultHeading='searchEngine'
         addHoverClass onClick={this.hoverCallback.bind(this)} />
       <div className='sectionTitle' data-l10n-id='locationBarSettings' />
       <SettingsList>
diff --git a/js/components/sortableTable.js b/js/components/sortableTable.js
index c41a787f0d4..701eedc3538 100644
--- a/js/components/sortableTable.js
+++ b/js/components/sortableTable.js
@@ -5,6 +5,7 @@
 const React = require('react')
 const ImmutableComponent = require('./immutableComponent')
 const tableSort = require('tablesort')
+const cx = require('../lib/classSet')
 
 /**
  * Represents a sortable table with supp
@@ -28,16 +29,14 @@ class SortableTable extends ImmutableComponent {
     return typeof this.props.onContextMenu === 'function' &&
       typeof this.props.contextMenuName === 'string'
   }
-  getHandlerInput (rows, index) {
-    if (this.props.rowObjects) {
-      return typeof this.props.rowObjects[index].toJS === 'function'
-        ? this.props.rowObjects[index].toJS()
-        : this.props.rowObjects[index]
-    }
-    return rows[index]
-  }
-  getRowAttributes (handlerInput, index) {
+  getRowAttributes (row, index) {
     const rowAttributes = {}
+    const handlerInput = this.props.rowObjects
+      ? (typeof this.props.rowObjects[index].toJS === 'function'
+        ? this.props.rowObjects[index].toJS()
+        : this.props.rowObjects[index])
+      : row
+
     if (this.props.addHoverClass) {
       rowAttributes.className = 'rowHover'
     }
@@ -53,50 +52,45 @@ class SortableTable extends ImmutableComponent {
     return rowAttributes
   }
   render () {
-    let headings = []
-    let rows = []
-    let columnClassNames = []
-
     if (!this.props.headings || !this.props.rows) {
       return false
     }
 
-    if (this.hasColumnClassNames) {
-      this.props.columnClassNames.forEach((className) => columnClassNames.push(className))
-    }
-
-    for (let i = 0; i < this.props.rows.length; i++) {
-      rows[i] = []
-      for (let j = 0; j < this.props.headings.length; j++) {
-        headings[j] = headings[j] || <th className='sort-header' data-l10n-id={this.props.headings[j]} />
-        rows[i][j] = typeof columnClassNames[j] === 'string'
-          ? <td className={columnClassNames[j]} data-sort={this.props.rows[i][j]}>{this.props.rows[i][j] === true ? '✕' : this.props.rows[i][j]}</td>
-          : <td data-sort={this.props.rows[i][j]}>{this.props.rows[i][j] === true ? '✕' : this.props.rows[i][j]}</td>
-      }
-
-      const handlerInput = this.getHandlerInput(rows, i)
-      const rowAttributes = this.getRowAttributes(handlerInput, i)
-
-      rows[i] = rowAttributes.onContextMenu
-      ? <tr {...rowAttributes} data-context-menu-disable>{rows[i]}</tr>
-      : rows[i] = <tr {...rowAttributes}>{rows[i]}</tr>
-    }
     return <table className='sortableTable sort'>
       <thead>
         <tr>
-          {headings}
+          {this.props.headings.map((heading) => <th className={cx({
+            'sort-header': true,
+            'sort-default': heading === this.props.defaultHeading
+          })}
+            data-l10n-id={heading}
+            data-sort-order={this.props.defaultHeadingSortOrder} />)}
         </tr>
       </thead>
       <tbody>
-        {rows}
+        {
+          this.props.rows.map((row, i) => {
+            const entry = row.map((item, j) => {
+              const value = typeof item === 'object' ? item.value : item
+              const html = typeof item === 'object' ? item.html : item
+              return <td className={this.hasColumnClassNames ? this.props.columnClassNames[j] : undefined} data-sort={value}>
+                {value === true ? '✕' : html}
+              </td>
+            })
+            const rowAttributes = this.getRowAttributes(row, i)
+            return rowAttributes.onContextMenu
+              ? <tr {...rowAttributes} data-context-menu-disable>{entry}</tr>
+              : <tr {...rowAttributes}>{entry}</tr>
+          })
+        }
       </tbody>
     </table>
   }
 }
 
 SortableTable.defaultProps = {
-  headings: React.PropTypes.array.isRequired,
-  rows: React.PropTypes.array.isRequired,
+  headings: React.PropTypes.array.isRequired, // list of data-10n-id's
+  rows: React.PropTypes.array.isRequired, // value or {html: <displayed_html>, value: <value_to_sort_by>} for each table entry
   columnClassNames: React.PropTypes.array,
   addHoverClass: React.PropTypes.bool,
   contextMenuName: React.PropTypes.string
diff --git a/less/sortableTable.less b/less/sortableTable.less
index fc4f2152b76..5007ec2107e 100644
--- a/less/sortableTable.less
+++ b/less/sortableTable.less
@@ -17,11 +17,17 @@ table.sortableTable {
 
     th {
       background: @lightGray;
+      cursor: pointer;
+      text-decoration: underline;
       color: @darkGray;
       text-align: left;
       font-weight: 300;
       padding: 8px;
       box-sizing: border-box;
+
+      &:hover {
+        color: #000;
+      }
     }
 
     td {