Feature/table refactor #38
Conversation
…nd parameter to storiesOf for ArrowLeft
…dings 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
This is proposed padding to fix our issue #7 . If it's acceptable we could use the same on govuk-react.
… and the name for values <td>
…her rowIncludes heading or vice versa
…ticalAlign style override test
Codecov Report
@@ Coverage Diff @@
## master #38 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 19 19
Lines 158 168 +10
Branches 21 24 +3
=====================================
+ Hits 158 168 +10
Continue to review full report at Codecov.
|
| @@ -86,7 +93,7 @@ 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 | |||
| <TableHeading key={title.key || index} name={names[index]}> | |||
There was a problem hiding this comment.
i'd prefer this to remain as it was
whilst the use-case for headings being named may be slightly dubious, we shouldn't prevent that from happening
components/table/src/index.js
Outdated
| ); | ||
|
|
||
| const getName = (names, row, column, rowIncludesHeading) => { | ||
| return (rowIncludesHeading) ? names.values[row] : names.values[column]; |
There was a problem hiding this comment.
changing the mode of operation of names so that it's now an object rather than an array is very inconvenient - we have dozens of uses of this component that are expecting the array style - we should change the format of names back so that it's expecting a simple array again
There was a problem hiding this comment.
we may also wish to consider using a different prop than rowIncludesHeading to indicate that our use of names should be based on vertical arrangement - it may be the case that we're including headings on rows, but that we still have multiple columns of data that should get differing names based on column number
perhaps this should be a boolean called nameByRow
i think that would work very nicely for the majority of cases
for a minority use-case (when nameByRow is false) the other idea we had of using a 2-d array of names would allow us to provide explicit names for every single cell on the table. the usefulness of this however is slightly dubious
There was a problem hiding this comment.
additionally with the current code this will fail if the names object doesn't include a values array
components/table/src/index.js
Outdated
| @@ -99,13 +106,13 @@ const Table = ({ name, names = [], rowIncludesHeading, titles, rows, flexibleCol | |||
| {row.map( | |||
| (item, itemIndex) => | |||
| rowIncludesHeading && itemIndex === 0 ? ( | |||
| <TableHeading rowHeading columnCount={row.length} key={item.key || itemIndex}> | |||
| <TableHeading rowHeading columnCount={row.length} key={item.key || itemIndex} verticalAlign={verticalAlign} name={names.headings}> | |||
There was a problem hiding this comment.
not sure about the wisdom or usefulness of this version of name here
arguably it should use getName
components/table/src/index.js
Outdated
| @@ -119,10 +126,24 @@ const Table = ({ name, names = [], rowIncludesHeading, titles, rows, flexibleCol | |||
| Table.propTypes = { | |||
| flexibleColumns: PropTypes.bool, | |||
| name: PropTypes.string, | |||
| names: PropTypes.arrayOf(PropTypes.string), | |||
| names: PropTypes.shape({ | |||
There was a problem hiding this comment.
as noted above, this change will break every place where this component is used
components/table/src/index.js
Outdated
| }); | ||
| })( | ||
| cellStyles, | ||
| ({ verticalAlign }) => ({ verticalAlign }), |
There was a problem hiding this comment.
i'm not entirely sure/convinced about allowing verticalAlign to be explicitly set and/or passed through
having a default style of verticalAlign: 'top' i think is OK for our current use-cases
…n: ‘top’ in emotion styles.
- 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
Update inline documentation for table and regenerate doc files
components/table/src/index.js
Outdated
| @@ -107,13 +151,13 @@ const Table = ({ name, names, rowIncludesHeading, titles, rows, flexibleColumns, | |||
| {row.map( | |||
| (item, itemIndex) => | |||
| rowIncludesHeading && itemIndex === 0 ? ( | |||
| <TableHeading rowHeading columnCount={row.length} key={item.key || itemIndex} verticalAlign={verticalAlign} name={names.headings}> | |||
| <TableHeading rowHeading columnCount={row.length} key={item.key || itemIndex} name={getName(names, calculateIndex(titles, nameByRow, index), itemIndex, nameByRow)}> | |||
There was a problem hiding this comment.
this line is far too wide and should be triggering a linting error as it's over 120 chars long 😁
There was a problem hiding this comment.
Let's make a call on #5 when we can. Also on whether to introduce Prettier. Maybe Ali is looking at this side of things. I think Prettier may be worth it but we'd need to be prepared to see a few commits which just (or also!) change formatting.
CHANGELOG:
This closes #7 and closes #9