From c3b2fa0c029e5002e16eb8345e005c01327fd3b8 Mon Sep 17 00:00:00 2001 From: Caroline Taymor and Kenny Wang Date: Wed, 15 Jul 2015 17:57:39 -0700 Subject: [PATCH] fix(sortable-table): sorts on first sortable column instead of on the first column regardless of whether it was sortable. Also refactors to use mergeprops. [Finishes #97658788] --- .../sortable-table/sortable-table_spec.js | 80 ++++++++++--------- .../sortable-table/sortable-table.js | 12 ++- src/pivotal-ui/javascripts/components.js | 3 +- 3 files changed, 54 insertions(+), 41 deletions(-) diff --git a/spec/pivotal-ui-react/sortable-table/sortable-table_spec.js b/spec/pivotal-ui-react/sortable-table/sortable-table_spec.js index 9e94b2c13..465c653f7 100644 --- a/spec/pivotal-ui-react/sortable-table/sortable-table_spec.js +++ b/spec/pivotal-ui-react/sortable-table/sortable-table_spec.js @@ -7,17 +7,20 @@ describe('SortableTable', function() { const data = [ { instances: '1', + bar: '9', title: 'foo', unsortable: '14' }, { instances: '3', + bar: '7', title: 'sup', unsortable: '22' }, { title: 'yee', instances: '2', + bar: '8', unsortable: '1' } ]; @@ -25,8 +28,9 @@ describe('SortableTable', function() { function renderSortableTable(data, props = {}) { clickSpy = jasmine.createSpy('click'); headers = [ - Title, - Instances, + Title, + Instances, + Bar, Unsortable ]; @@ -37,6 +41,7 @@ describe('SortableTable', function() { {datum.title} {datum.instances} + {datum.bar} {datum.unsortable} ); @@ -56,7 +61,7 @@ describe('SortableTable', function() { }); it('adds the class "sortable" on all sortable columns', function() { - expect('th:contains("Title")').toHaveClass('sortable'); + expect('th:contains("Title")').not.toHaveClass('sortable'); expect('th:contains("Instances")').toHaveClass('sortable'); expect('th:contains("Unsortable")').not.toHaveClass('sortable'); }); @@ -70,21 +75,21 @@ describe('SortableTable', function() { expect('table.table-sortable').toHaveCss({opacity: '0.5'}); }); - it('sorts table rows by the first column in ascending order by default', function() { - expect('th:contains("Title")').toHaveClass('sorted-asc'); + it('sorts table rows by the first sortable column in ascending order by default', function() { + expect('th:contains("Instances")').toHaveClass('sorted-asc'); expect('tbody tr:nth-of-type(1) > td:eq(0)').toContainText('foo'); - expect('tbody tr:nth-of-type(2) > td:eq(0)').toContainText('sup'); - expect('tbody tr:nth-of-type(3) > td:eq(0)').toContainText('yee'); + expect('tbody tr:nth-of-type(2) > td:eq(0)').toContainText('yee'); + expect('tbody tr:nth-of-type(3) > td:eq(0)').toContainText('sup'); expect('tbody tr:nth-of-type(1) > td:eq(1)').toContainText('1'); - expect('tbody tr:nth-of-type(2) > td:eq(1)').toContainText('3'); - expect('tbody tr:nth-of-type(3) > td:eq(1)').toContainText('2'); + expect('tbody tr:nth-of-type(2) > td:eq(1)').toContainText('2'); + expect('tbody tr:nth-of-type(3) > td:eq(1)').toContainText('3'); }); describe('clicking on the already asc-sorted column that has an existing onClick function', function() { beforeEach(function() { - $('th:contains("Title")').simulate('click'); + $('th:contains("Instances")').simulate('click'); }); it('calls the onClick function', function() { @@ -92,21 +97,21 @@ describe('SortableTable', function() { }); it('reverses the sort order', function() { - expect('th:contains("Title")').toHaveClass('sorted-desc'); + expect('th:contains("Instances")').toHaveClass('sorted-desc'); - expect('tbody tr:nth-of-type(1) > td:eq(0)').toContainText('yee'); - expect('tbody tr:nth-of-type(2) > td:eq(0)').toContainText('sup'); + expect('tbody tr:nth-of-type(1) > td:eq(0)').toContainText('sup'); + expect('tbody tr:nth-of-type(2) > td:eq(0)').toContainText('yee'); expect('tbody tr:nth-of-type(3) > td:eq(0)').toContainText('foo'); - expect('tbody tr:nth-of-type(1) > td:eq(1)').toContainText('2'); - expect('tbody tr:nth-of-type(2) > td:eq(1)').toContainText('3'); + expect('tbody tr:nth-of-type(1) > td:eq(1)').toContainText('3'); + expect('tbody tr:nth-of-type(2) > td:eq(1)').toContainText('2'); expect('tbody tr:nth-of-type(3) > td:eq(1)').toContainText('1'); }); describe('clicking on the already desc-sorted column', function() { beforeEach(function() { clickSpy.calls.reset(); - $('th:contains("Title")').simulate('click'); + $('th:contains("Instances")').simulate('click'); }); it('calls the onClick function', function() { @@ -114,35 +119,35 @@ describe('SortableTable', function() { }); it('reverses the sort order', function() { - expect('th:contains("Title")').toHaveClass('sorted-asc'); + expect('th:contains("Instances")').toHaveClass('sorted-asc'); expect('tbody tr:nth-of-type(1) > td:eq(0)').toContainText('foo'); - expect('tbody tr:nth-of-type(2) > td:eq(0)').toContainText('sup'); - expect('tbody tr:nth-of-type(3) > td:eq(0)').toContainText('yee'); + expect('tbody tr:nth-of-type(2) > td:eq(0)').toContainText('yee'); + expect('tbody tr:nth-of-type(3) > td:eq(0)').toContainText('sup'); expect('tbody tr:nth-of-type(1) > td:eq(1)').toContainText('1'); - expect('tbody tr:nth-of-type(2) > td:eq(1)').toContainText('3'); - expect('tbody tr:nth-of-type(3) > td:eq(1)').toContainText('2'); + expect('tbody tr:nth-of-type(2) > td:eq(1)').toContainText('2'); + expect('tbody tr:nth-of-type(3) > td:eq(1)').toContainText('3'); }); }); }); describe('clicking on a sortable column', function() { beforeEach(function() { - $('th:contains("Instances")').simulate('click'); + $('th:contains("Bar")').simulate('click'); }); it('sorts table rows by that column', function() { - expect('th:contains("Instances")').toHaveClass('sorted-asc'); + expect('th:contains("Bar")').toHaveClass('sorted-asc'); expect('th:contains("Title")').not.toHaveClass('sorted-asc'); - expect('tbody tr:nth-of-type(1) > td:eq(0)').toContainText('foo'); + expect('tbody tr:nth-of-type(1) > td:eq(0)').toContainText('sup'); expect('tbody tr:nth-of-type(2) > td:eq(0)').toContainText('yee'); - expect('tbody tr:nth-of-type(3) > td:eq(0)').toContainText('sup'); + expect('tbody tr:nth-of-type(3) > td:eq(0)').toContainText('foo'); - expect('tbody tr:nth-of-type(1) > td:eq(1)').toContainText('1'); - expect('tbody tr:nth-of-type(2) > td:eq(1)').toContainText('2'); - expect('tbody tr:nth-of-type(3) > td:eq(1)').toContainText('3'); + expect('tbody tr:nth-of-type(1) > td:eq(2)').toContainText('7'); + expect('tbody tr:nth-of-type(2) > td:eq(2)').toContainText('8'); + expect('tbody tr:nth-of-type(3) > td:eq(2)').toContainText('9'); }); }); @@ -153,15 +158,15 @@ describe('SortableTable', function() { it('does not change the sort', function() { expect('th:contains("Unsortable")').not.toHaveClass('sorted-asc'); - expect('th:contains("Title")').toHaveClass('sorted-asc'); + expect('th:contains("Instances")').toHaveClass('sorted-asc'); expect('tbody tr:nth-of-type(1) > td:eq(0)').toContainText('foo'); - expect('tbody tr:nth-of-type(2) > td:eq(0)').toContainText('sup'); - expect('tbody tr:nth-of-type(3) > td:eq(0)').toContainText('yee'); + expect('tbody tr:nth-of-type(2) > td:eq(0)').toContainText('yee'); + expect('tbody tr:nth-of-type(3) > td:eq(0)').toContainText('sup'); expect('tbody tr:nth-of-type(1) > td:eq(1)').toContainText('1'); - expect('tbody tr:nth-of-type(2) > td:eq(1)').toContainText('3'); - expect('tbody tr:nth-of-type(3) > td:eq(1)').toContainText('2'); + expect('tbody tr:nth-of-type(2) > td:eq(1)').toContainText('2'); + expect('tbody tr:nth-of-type(3) > td:eq(1)').toContainText('3'); }); }); @@ -169,16 +174,17 @@ describe('SortableTable', function() { beforeEach(function() { var newData = data.concat({ title: 'new title', - instances: '3', - unsortable: '1' + instances: '1.5', + unsortable: '1', + bar: '123' }); renderSortableTable(newData); }); it('shows the new rows in the correct sort order', function() { expect('tbody tr').toHaveLength(4); - var titles = $('tbody tr > td:first-child').map(function() {return $(this).text(); }).toArray(); - expect(titles).toEqual(['foo', 'new title', 'sup', 'yee']); + var instances = $('tbody tr > td:nth-of-type(2)').map(function() {return $(this).text(); }).toArray(); + expect(instances).toEqual(['1', '1.5', '2', '3']); }); }); }); diff --git a/src/pivotal-ui-react/sortable-table/sortable-table.js b/src/pivotal-ui-react/sortable-table/sortable-table.js index 162c4d8bd..4865aed7a 100644 --- a/src/pivotal-ui-react/sortable-table/sortable-table.js +++ b/src/pivotal-ui-react/sortable-table/sortable-table.js @@ -1,6 +1,8 @@ const classnames = require('classnames'); const React = require('react'); const sortBy = require('lodash.sortby'); +import {mergeProps} from '../../../src/pivotal-ui-react/helpers/helpers'; + const types = React.PropTypes; @@ -96,7 +98,11 @@ export const SortableTable = React.createClass({ }, getInitialState() { - return {sortColumn: 0, sortAscending: true}; + const sortCol = this.props.headers.findIndex((header) => { + return header.props.sortable; + }); + // If none of the columns are sortable we default to the 0th column + return {sortColumn: sortCol === -1 ? 0 : sortCol, sortAscending: true}; }, setSortedColumn(sortColumn, sortAscending) { @@ -138,11 +144,11 @@ export const SortableTable = React.createClass({ }, render() { - let {className, style, id} = this.props; + const props = mergeProps(this.props, {className: ['table', 'table-sortable']}); let rows = this.sortedRows(); return ( - +
{this.renderHeaders()} diff --git a/src/pivotal-ui/javascripts/components.js b/src/pivotal-ui/javascripts/components.js index 82011b5f1..027e818b3 100644 --- a/src/pivotal-ui/javascripts/components.js +++ b/src/pivotal-ui/javascripts/components.js @@ -118,4 +118,5 @@ module.exports = { BackToTop: require('pui-react-back-to-top').BackToTop, PortalSource: require('pui-react-portals').PortalSource, - PortalDestination: require('pui-react-portals').PortalDestination, ...sortableTable}; \ No newline at end of file + PortalDestination: require('pui-react-portals').PortalDestination, + ...sortableTable};