From 060244eb9fab4c7acc7417c03543e396563359c1 Mon Sep 17 00:00:00 2001 From: Tarandeep Singh Chauhan Date: Mon, 6 Aug 2018 11:53:25 +0100 Subject: [PATCH] Feature/table refactor (#38) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Correct syntax error for Hot module replacement to add module as second parameter to storiesOf for ArrowLeft * Pass Multi dimensional array into names so that rows that include headings are grouped horizontally instead of vertically e.g. first row of values are all named row ‘one’ Headings for Column names are still named incrementally e.g. one, two, three, four * Remove duplicate styling and adjust paddings This is proposed padding to fix our issue https://github.com/stevesims/govuk-frederic/issues/7 . If it's acceptable we could use the same on govuk-react. * Add vertical align style optional style override. * Correct deletion of space between rowHeading and columnCount. * Generate docs * Fix verticalAlign props passthrough * Refactor stories to pass in an object with the name for headings and the name for values * Refactor getName component to calculate th and td names based on whether rowIncludes heading or vice versa * Refactor existing tests and add new tests based on new props e.g. verticalAlign style override test * Update docs for Table * Remove default parameters * Remove console.log from test * Remove verticalAlign as prop pass through and default to verticalAlign: ‘top’ in emotion styles. * - Make names an array again - Add nameByRow prop to decide if naming should be applied across columns or rows - Ensure names array allows for entire rows/columns to be named the same or cells can have completely individual names * Refactor stories based on names array * Refactor tests based on names array * Regenerate documentation * Update Table usage: Update inline documentation for table and regenerate doc files * Fix TableHeading line lengths greater than 100 chars. --- API.md | 57 +++++++- components/table/README.md | 57 +++++++- .../table/src/__snapshots__/test.js.snap | 5 +- components/table/src/index.js | 138 +++++++++++++----- components/table/src/stories.js | 19 ++- components/table/src/test.js | 77 +++++++++- 6 files changed, 288 insertions(+), 65 deletions(-) diff --git a/API.md b/API.md index 9725bf6..0f72501 100644 --- a/API.md +++ b/API.md @@ -893,36 +893,77 @@ Table Simple ```jsx - +const arrayExampleHeadings = ['Heading 1', 'Heading 2', 'Heading 3', 'Heading 4']; +const arrayExampleContent = [ + ['Content 1-1', 'Content 1-2', 'Content 1-3', 'Content 1-4'], + ['Content 2-1', 'Content 2-2', 'Content 2-3', 'Content 2-4'], + ['Content 3-1', 'Content 3-2', 'Content 3-3', 'Content 3-4'], +]; +const columnTableNames = ['one', 'two', 'three', ['i', 'am', 'named', 'individually']]; + +
``` rowIncludesHeading ```jsx -
+const arrayExampleHeadings = ['Heading 1', 'Heading 2', 'Heading 3', 'Heading 4']; +const arrayExampleContent = [ + ['Content 1-1', 'Content 1-2', 'Content 1-3', 'Content 1-4'], + ['Content 2-1', 'Content 2-2', 'Content 2-3', 'Content 2-4'], + ['Content 3-1', 'Content 3-2', 'Content 3-3', 'Content 3-4'], +]; +const rowTableNamesWithTitles = ['heading', 'one', ['i', 'am', 'named', 'individually'], 'three']; + +
``` rowIncludesHeading, no titles ```jsx -
+const arrayExampleHeadings = ['Heading 1', 'Heading 2', 'Heading 3', 'Heading 4']; +const arrayExampleContent = [ + ['Content 1-1', 'Content 1-2', 'Content 1-3', 'Content 1-4'], + ['Content 2-1', 'Content 2-2', 'Content 2-3', 'Content 2-4'], + ['Content 3-1', 'Content 3-2', 'Content 3-3', 'Content 3-4'], +]; +const rowTableNames = ['one', ['i', 'am', 'named', 'individually'], 'three']; + +
``` rowIncludesHeading, no titles, small single row ```jsx -
+const arrayExampleHeadings = ['Heading 1', 'Heading 2', 'Heading 3', 'Heading 4']; +const arrayExampleContent = [ + ['Content 1-1', 'Content 1-2', 'Content 1-3', 'Content 1-4'], + ['Content 2-1', 'Content 2-2', 'Content 2-3', 'Content 2-4'], + ['Content 3-1', 'Content 3-2', 'Content 3-3', 'Content 3-4'], +]; +const rowTableNames = ['one', ['i', 'am', 'named', 'individually'], 'three']; + +
``` -rowIncludesHeading, with flexible columns +rowIncludesHeading, with titles, with flexible columns ```jsx -
+const arrayExampleHeadings = ['Heading 1', 'Heading 2', 'Heading 3', 'Heading 4']; +const arrayExampleContent = [ + ['Content 1-1', 'Content 1-2', 'Content 1-3', 'Content 1-4'], + ['Content 2-1', 'Content 2-2', 'Content 2-3', 'Content 2-4'], + ['Content 3-1', 'Content 3-2', 'Content 3-3', 'Content 3-4'], +]; +const rowTableNamesWithTitles = ['heading', 'one', ['i', 'am', 'named', 'individually'], 'three']; + +
``` ### Properties Prop | Required | Default | Type | Description :--- | :------- | :------ | :--- | :---------- - `flexibleColumns` | | `````` | bool | + `flexibleColumns` | | ```false``` | bool | `name` | | `````` | string | + `nameByRow` | | ```false``` | bool | `names` | | ```[]``` | arrayOf[object Object] | - `rowIncludesHeading` | | `````` | bool | + `rowIncludesHeading` | | ```false``` | bool | `rows` | true | `````` | arrayOf[object Object] | `titles` | | `````` | arrayOf[object Object] | diff --git a/components/table/README.md b/components/table/README.md index 235daf8..c6674b6 100644 --- a/components/table/README.md +++ b/components/table/README.md @@ -11,36 +11,77 @@ Table Simple ```jsx -
+const arrayExampleHeadings = ['Heading 1', 'Heading 2', 'Heading 3', 'Heading 4']; +const arrayExampleContent = [ + ['Content 1-1', 'Content 1-2', 'Content 1-3', 'Content 1-4'], + ['Content 2-1', 'Content 2-2', 'Content 2-3', 'Content 2-4'], + ['Content 3-1', 'Content 3-2', 'Content 3-3', 'Content 3-4'], +]; +const columnTableNames = ['one', 'two', 'three', ['i', 'am', 'named', 'individually']]; + +
``` rowIncludesHeading ```jsx -
+const arrayExampleHeadings = ['Heading 1', 'Heading 2', 'Heading 3', 'Heading 4']; +const arrayExampleContent = [ + ['Content 1-1', 'Content 1-2', 'Content 1-3', 'Content 1-4'], + ['Content 2-1', 'Content 2-2', 'Content 2-3', 'Content 2-4'], + ['Content 3-1', 'Content 3-2', 'Content 3-3', 'Content 3-4'], +]; +const rowTableNamesWithTitles = ['heading', 'one', ['i', 'am', 'named', 'individually'], 'three']; + +
``` rowIncludesHeading, no titles ```jsx -
+const arrayExampleHeadings = ['Heading 1', 'Heading 2', 'Heading 3', 'Heading 4']; +const arrayExampleContent = [ + ['Content 1-1', 'Content 1-2', 'Content 1-3', 'Content 1-4'], + ['Content 2-1', 'Content 2-2', 'Content 2-3', 'Content 2-4'], + ['Content 3-1', 'Content 3-2', 'Content 3-3', 'Content 3-4'], +]; +const rowTableNames = ['one', ['i', 'am', 'named', 'individually'], 'three']; + +
``` rowIncludesHeading, no titles, small single row ```jsx -
+const arrayExampleHeadings = ['Heading 1', 'Heading 2', 'Heading 3', 'Heading 4']; +const arrayExampleContent = [ + ['Content 1-1', 'Content 1-2', 'Content 1-3', 'Content 1-4'], + ['Content 2-1', 'Content 2-2', 'Content 2-3', 'Content 2-4'], + ['Content 3-1', 'Content 3-2', 'Content 3-3', 'Content 3-4'], +]; +const rowTableNames = ['one', ['i', 'am', 'named', 'individually'], 'three']; + +
``` -rowIncludesHeading, with flexible columns +rowIncludesHeading, with titles, with flexible columns ```jsx -
+const arrayExampleHeadings = ['Heading 1', 'Heading 2', 'Heading 3', 'Heading 4']; +const arrayExampleContent = [ + ['Content 1-1', 'Content 1-2', 'Content 1-3', 'Content 1-4'], + ['Content 2-1', 'Content 2-2', 'Content 2-3', 'Content 2-4'], + ['Content 3-1', 'Content 3-2', 'Content 3-3', 'Content 3-4'], +]; +const rowTableNamesWithTitles = ['heading', 'one', ['i', 'am', 'named', 'individually'], 'three']; + +
``` ### Properties Prop | Required | Default | Type | Description :--- | :------- | :------ | :--- | :---------- - `flexibleColumns` | | `````` | bool | + `flexibleColumns` | | ```false``` | bool | `name` | | `````` | string | + `nameByRow` | | ```false``` | bool | `names` | | ```[]``` | arrayOf[object Object] | - `rowIncludesHeading` | | `````` | bool | + `rowIncludesHeading` | | ```false``` | bool | `rows` | true | `````` | arrayOf[object Object] | `titles` | | `````` | arrayOf[object Object] | diff --git a/components/table/src/__snapshots__/test.js.snap b/components/table/src/__snapshots__/test.js.snap index 00a3dd1..1e52920 100644 --- a/components/table/src/__snapshots__/test.js.snap +++ b/components/table/src/__snapshots__/test.js.snap @@ -9,7 +9,7 @@ exports[`Table matches snapshot 1`] = ` Heading 4 @@ -22,6 +22,7 @@ exports[`Table matches snapshot 1`] = ` Content 1-4 @@ -33,6 +34,7 @@ exports[`Table matches snapshot 1`] = ` Content 2-4 @@ -44,6 +46,7 @@ exports[`Table matches snapshot 1`] = ` Content 3-4 diff --git a/components/table/src/index.js b/components/table/src/index.js index 1c0ecdb..8316b7f 100644 --- a/components/table/src/index.js +++ b/components/table/src/index.js @@ -15,39 +15,50 @@ const TableContainer = styled('table', { ({ flexibleColumns }) => ({ tableLayout: flexibleColumns ? 'auto' : 'fixed' }), ); -const TableHeading = styled('th', { - forwardProps: ['name'], -})(({ rowHeading, columnCount }) => ({ +const cellStyles = { ':first-child': { - padding: '15px 8px 15px 0', - width: rowHeading && columnCount < 4 ? '25%' : undefined, + padding: '15px 4px 15px 0', }, ':last-child': { - padding: '15px 0 15px 8px', + padding: '15px 0 15px 4px', }, borderBottom: '1px solid #bfc1c3', display: 'table-cell', fontSize: '14px', - fontWeight: 'bold', - textAlign: 'left', - verticalAlign: 'baseline', -})); + padding: '15px 4px', + verticalAlign: 'top', +}; const TableData = styled('td', { // use `forwardProps` here as by default emotion doesn't allow setting `name` prop on a `td` forwardProps: ['name'], -})({ - ':first-child': { - padding: '15px 8px 15px 0', - }, - ':last-child': { - padding: '15px 0 15px 8px', - }, - borderBottom: '1px solid #bfc1c3', - display: 'table-cell', - fontSize: '14px', - verticalAlign: 'baseline', -}); +})(cellStyles); + +const TableHeading = styled('th')( + cellStyles, + ({rowHeading, columnCount}) => ( + { + fontWeight: 'bold', + textAlign: 'left', + width: rowHeading && columnCount < 4 ? '25%' : undefined, + } + ), +); + +const getName = (names, row, column, nameByRow) => { + if (nameByRow) { + return Array.isArray(names[row]) ? names[row][column] : names[row]; + } + return Array.isArray(names[column]) ? names[column][row] : names[column]; +}; + +const calculateIndex = (titles, nameByRow, index) => { + if (nameByRow) { + // Only if there are headings at the top, we need to increment the row + return (titles && titles.length) ? (index + 1) : index; + } + return (index + 1); +}; /** * @@ -55,30 +66,70 @@ const TableData = styled('td', { * * Simple * ```jsx - *
+ * const arrayExampleHeadings = ['Heading 1', 'Heading 2', 'Heading 3', 'Heading 4']; + * const arrayExampleContent = [ + * ['Content 1-1', 'Content 1-2', 'Content 1-3', 'Content 1-4'], + * ['Content 2-1', 'Content 2-2', 'Content 2-3', 'Content 2-4'], + * ['Content 3-1', 'Content 3-2', 'Content 3-3', 'Content 3-4'], + * ]; + * const columnTableNames = ['one', 'two', 'three', ['i', 'am', 'named', 'individually']]; + * + *
* ``` * * rowIncludesHeading * ```jsx - *
+ * const arrayExampleHeadings = ['Heading 1', 'Heading 2', 'Heading 3', 'Heading 4']; + * const arrayExampleContent = [ + * ['Content 1-1', 'Content 1-2', 'Content 1-3', 'Content 1-4'], + * ['Content 2-1', 'Content 2-2', 'Content 2-3', 'Content 2-4'], + * ['Content 3-1', 'Content 3-2', 'Content 3-3', 'Content 3-4'], + * ]; + * const rowTableNamesWithTitles = ['heading', 'one', ['i', 'am', 'named', 'individually'], 'three']; + * + *
* ``` * * rowIncludesHeading, no titles * ```jsx - *
+ * const arrayExampleHeadings = ['Heading 1', 'Heading 2', 'Heading 3', 'Heading 4']; + * const arrayExampleContent = [ + * ['Content 1-1', 'Content 1-2', 'Content 1-3', 'Content 1-4'], + * ['Content 2-1', 'Content 2-2', 'Content 2-3', 'Content 2-4'], + * ['Content 3-1', 'Content 3-2', 'Content 3-3', 'Content 3-4'], + * ]; + * const rowTableNames = ['one', ['i', 'am', 'named', 'individually'], 'three']; + * + *
* ``` * * rowIncludesHeading, no titles, small single row * ```jsx - *
+ * const arrayExampleHeadings = ['Heading 1', 'Heading 2', 'Heading 3', 'Heading 4']; + * const arrayExampleContent = [ + * ['Content 1-1', 'Content 1-2', 'Content 1-3', 'Content 1-4'], + * ['Content 2-1', 'Content 2-2', 'Content 2-3', 'Content 2-4'], + * ['Content 3-1', 'Content 3-2', 'Content 3-3', 'Content 3-4'], + * ]; + * const rowTableNames = ['one', ['i', 'am', 'named', 'individually'], 'three']; + * + *
* ``` * - * rowIncludesHeading, with flexible columns + * rowIncludesHeading, with titles, with flexible columns * ```jsx - *
+ * const arrayExampleHeadings = ['Heading 1', 'Heading 2', 'Heading 3', 'Heading 4']; + * const arrayExampleContent = [ + * ['Content 1-1', 'Content 1-2', 'Content 1-3', 'Content 1-4'], + * ['Content 2-1', 'Content 2-2', 'Content 2-3', 'Content 2-4'], + * ['Content 3-1', 'Content 3-2', 'Content 3-3', 'Content 3-4'], + * ]; + * const rowTableNamesWithTitles = ['heading', 'one', ['i', 'am', 'named', 'individually'], 'three']; + * + *
* ``` */ -const Table = ({ name, names = [], rowIncludesHeading, titles, rows, flexibleColumns }) => ( +const Table = ({ name, names, rowIncludesHeading, nameByRow, titles, rows, flexibleColumns }) => ( {titles && titles.length && ( @@ -87,7 +138,9 @@ const Table = ({ name, names = [], rowIncludesHeading, titles, rows, flexibleCol {titles.map((title, index) => ( // disable false-positive rule - this is an access into an array of strings, not object access // eslint-disable-next-line security/detect-object-injection - + {title} ))} @@ -100,13 +153,19 @@ const Table = ({ name, names = [], rowIncludesHeading, titles, rows, flexibleCol {row.map( (item, itemIndex) => rowIncludesHeading && itemIndex === 0 ? ( - + {item} ) : ( // disable false-positive rule - this is an access into an array of strings, not object access // eslint-disable-next-line security/detect-object-injection - + {item} ), @@ -120,10 +179,23 @@ const Table = ({ name, names = [], rowIncludesHeading, titles, rows, flexibleCol Table.propTypes = { flexibleColumns: PropTypes.bool, name: PropTypes.string, - names: PropTypes.arrayOf(PropTypes.string), + names: PropTypes.arrayOf( + PropTypes.oneOfType([ + PropTypes.string, + PropTypes.arrayOf(PropTypes.string), + ]), + ), + nameByRow: PropTypes.bool, rowIncludesHeading: PropTypes.bool, rows: PropTypes.arrayOf(PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.string, PropTypes.node]))).isRequired, titles: PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.string, PropTypes.node])), }; +Table.defaultProps = { + flexibleColumns: false, + nameByRow: false, + names: [], + rowIncludesHeading: false, +}; + export default Table; diff --git a/components/table/src/stories.js b/components/table/src/stories.js index 22b7e43..a3a754b 100644 --- a/components/table/src/stories.js +++ b/components/table/src/stories.js @@ -8,11 +8,14 @@ import ReadMe from '../README.md'; const arrayExampleHeadings = ['Heading 1', 'Heading 2', 'Heading 3', 'Heading 4']; const arrayExampleContent = [ - ['Content 1-1', 'Content 1-2', 'Content 3', 'Content 4'], + ['Content 1-1', 'Content 1-2', 'Content 1-3', 'Content 1-4'], ['Content 2-1', 'Content 2-2', 'Content 2-3', 'Content 2-4'], ['Content 3-1', 'Content 3-2', 'Content 3-3', 'Content 3-4'], ]; -const exampleNames = ['one', 'two', 'three', 'four']; + +const columnTableNames = ['one', 'two', 'three', ['i', 'am', 'named', 'individually']]; +const rowTableNamesWithTitles = ['heading', 'one', ['i', 'am', 'named', 'individually'], 'three']; +const rowTableNames = ['one', ['i', 'am', 'named', 'individually'], 'three']; const stories = storiesOf('Tables/Table', module); const examples = storiesOf('Tables/Table/Examples', module); @@ -21,21 +24,21 @@ stories.addDecorator(WithDocsCustom(ReadMe)); stories.addDecorator(withKnobs); stories.add('Component default', () => -
, +
, ); -examples.add('rowIncludesHeading', () => -
, +examples.add('rowIncludesHeading, with titles', () => +
, ); examples.add('rowIncludesHeading, no titles', () => -
, +
, ); examples.add('rowIncludesHeading, no titles, small single row', () => -
, +
, ); examples.add('rowIncludesHeading, with flexible columns', () => -
, +
, ); diff --git a/components/table/src/test.js b/components/table/src/test.js index 9a135a4..5b1726e 100644 --- a/components/table/src/test.js +++ b/components/table/src/test.js @@ -14,7 +14,11 @@ describe('Table', () => { ['Content 2-1', 'Content 2-2', 'Content 2-3', 'Content 2-4'], ['Content 3-1', 'Content 3-2', 'Content 3-3', 'Content 3-4'], ]; - const names = ['one', 'two', 'three', 'four']; + + const columnTableNames = ['one', 'two', 'three', ['i', 'am', 'named', 'individually']]; + const rowTableNamesWithTitles = ['heading', 'one', ['i', 'am', 'named', 'individually'], 'three']; + const rowTableNames = ['one', ['i', 'am', 'named', 'individually'], 'three']; + let wrapper; it('renders as a table', () => { @@ -30,17 +34,76 @@ describe('Table', () => { expect(wrapper.find('tr')).toHaveLength(4); }); + it('names each cell according to its column', () => { + wrapper.setProps({name: 'name', names: columnTableNames}); + const th = wrapper.find('TableHeading').at(2); + expect(th.prop('name')).toBe('three'); + const td = wrapper.find('TableData').at(2); + expect(td.prop('name')).toBe('three'); + }); + it('renders rows with header column', () => { - wrapper.setProps({rowIncludesHeading: true}); + wrapper.setProps({rowIncludesHeading: true, nameByRow: true}); expect(wrapper.find('TableHeading')).toHaveLength(7); }); - it('names each cell according to its column', () => { - wrapper.setProps({name: 'name', names}); - const th = wrapper.find('TableHeading').at(2); + it('names each cell according to its row, no titles', () => { + wrapper.setProps({ titles: undefined, names: rowTableNames }); + + let th = wrapper.find('TableHeading').at(0); + expect(th.prop('name')).toBe('one'); + + let td = wrapper.find('TableData').at(0); + expect(td.prop('name')).toBe('one'); + + th = wrapper.find('TableHeading').at(1); + expect(th.prop('name')).toBe('i'); + + td = wrapper.find('TableData').at(3); + expect(td.prop('name')).toBe('am'); + + td = wrapper.find('TableData').at(4); + expect(td.prop('name')).toBe('named'); + + td = wrapper.find('TableData').at(5); + expect(td.prop('name')).toBe('individually'); + + th = wrapper.find('TableHeading').at(2); expect(th.prop('name')).toBe('three'); - const td = wrapper.find('TableData').at(2); - expect(td.prop('name')).toBe('four'); + + td = wrapper.find('TableData').at(8); + expect(td.prop('name')).toBe('three'); + }); + + it('names each cell according to its row, with titles', () => { + wrapper.setProps({ titles, names: rowTableNamesWithTitles }); + + let th = wrapper.find('TableHeading').at(3); + expect(th.prop('name')).toBe('heading'); + + th = wrapper.find('TableHeading').at(4); + expect(th.prop('name')).toBe('one'); + + let td = wrapper.find('TableData').at(0); + expect(td.prop('name')).toBe('one'); + + th = wrapper.find('TableHeading').at(5); + expect(th.prop('name')).toBe('i'); + + td = wrapper.find('TableData').at(3); + expect(td.prop('name')).toBe('am'); + + td = wrapper.find('TableData').at(4); + expect(td.prop('name')).toBe('named'); + + td = wrapper.find('TableData').at(5); + expect(td.prop('name')).toBe('individually'); + + th = wrapper.find('TableHeading').at(6); + expect(th.prop('name')).toBe('three'); + + td = wrapper.find('TableData').at(8); + expect(td.prop('name')).toBe('three'); }); it('sets th width according to rowHeading prop', () => {