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]
pivotal-cf · Jul 16, 2015 · c3b2fa0 · c3b2fa0
1 parent 537f477
commit c3b2fa0
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 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
@@ -7,26 +7,30 @@ 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'
  }
  ];
 
  function renderSortableTable(data, props = {}) {
  clickSpy = jasmine.createSpy('click');
  headers = [
- <TableHeader sortable={true} onClick={clickSpy}>Title</TableHeader>,
- <TableHeader sortable={true}>Instances</TableHeader>,
+ <TableHeader>Title</TableHeader>,
+ <TableHeader onClick={clickSpy} sortable={true}>Instances</TableHeader>,
+ <TableHeader sortable={true}>Bar</TableHeader>,
  <TableHeader>Unsortable</TableHeader>
  ];
 
@@ -37,6 +41,7 @@ describe('SortableTable', function() {
  <TableRow key={key}>
  <TableCell>{datum.title}</TableCell>
  <TableCell>{datum.instances}</TableCell>
+ <TableCell>{datum.bar}</TableCell>
  <TableCell>{datum.unsortable}</TableCell>
  </TableRow>
  );
@@ -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,79 +75,79 @@ 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() {
  expect(clickSpy).toHaveBeenCalled();
  });
 
  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() {
  expect(clickSpy).toHaveBeenCalled();
  });
 
  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,32 +158,33 @@ 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');
  });
  });
 
  describe('when the rows change', 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
@@ -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 (
- <table className={classnames('table', 'table-sortable', className)} id={id} style={style} >
+ <table {...props} >
  <thead>
  <tr>{this.renderHeaders()}</tr>
  </thead>

diff --git 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};
+ PortalDestination: require('pui-react-portals').PortalDestination,
+ ...sortableTable};