Issue 491 #498
Issue 491 #498
Conversation
src/dash-table/utils/actions.js
Outdated
| @@ -1,12 +1,20 @@ | |||
| import * as R from 'ramda'; | |||
| import merge from 'ramda/es/merge'; | |||
There was a problem hiding this comment.
Use R.merge instead
There was a problem hiding this comment.
R.merge is deprecated - mergeRight is its more explicit replacement.
There was a problem hiding this comment.
Interesting, that makes for a lot of R.merge usage to be updated in the table repo later :)
src/dash-table/utils/actions.js
Outdated
| const columnHeaders = columns.map(col => col.name); | ||
| const maxLength = findMaxLength(columnHeaders); | ||
| const newColumnNames = Array(maxLength).fill(column.name); | ||
| cloneColumn = Object.assign({}, column, {name: newColumnNames}); |
There was a problem hiding this comment.
No need to transform into an array if there is only one header row.
src/dash-table/utils/actions.js
Outdated
| cloneColumn = Object.assign({}, column, {name: newColumnNames}); | ||
| } | ||
| const transformedColumns = columns.map(col => { | ||
| if (((typeof column.name === 'string') || (column.name instanceof String)) && col.id === column.id) { |
There was a problem hiding this comment.
instanceof String is only useful if we use the new String(...), which we do not (enforced
through tslint no-construct rule), typeof should be enough.
src/dash-table/utils/actions.js
Outdated
| maxLength = row.length; | ||
| }}, array); | ||
| return maxLength; | ||
| } |
There was a problem hiding this comment.
src/dash-table/utils/actions.js
Outdated
| @@ -53,23 +61,36 @@ export const clearSelection = { | |||
| selected_cells: [] | |||
| }; | |||
|
|
|||
| export function editColumnName(column, columns, headerRowIndex) { | |||
| export function changeColumnHeader(column, columns, headerRowIndex, mergeDuplicateHeaders, newColumnName) { | |||
| let cloneColumn = Object.assign({}, column); | |||
There was a problem hiding this comment.
Use R.merge or R.mergeAll for consistency. The nice advantage is that it treats all arguments as immutables -- so you do not have to add a target { }.
src/dash-table/utils/actions.js
Outdated
| } else { | ||
| return col; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Instead of a map you can:
newColumns = columns.slice(0) // shallow clone of array
newColumns[newColumns.indexOf(column)] = newColumn // replace the modified entry
| }); | ||
| }); | ||
|
|
||
| describe.only(`edit headers, mode=${AppMode.Typed}`, () => { |
There was a problem hiding this comment.
I think a file for header tests is warranted.
Don't forget to remove the .only :)
src/dash-table/utils/actions.js
Outdated
| /* eslint no-alert: 0 */ | ||
| const newColumnName = window.prompt('Enter a new column name'); | ||
| let newColumns = R.clone(columns); | ||
| let newColumns = R.clone(transformedColumns); |
There was a problem hiding this comment.
I don't think the deep clone (https://ramdajs.com/docs/#clone) is necessary here. It will actually make multiple memoizeOne functions fail their test on their column argument, forcing a significant portion of the table to re-evaluate itself.
src/dash-table/utils/actions.js
Outdated
| @@ -53,23 +53,31 @@ export const clearSelection = { | |||
| selected_cells: [] | |||
| }; | |||
|
|
|||
| export function editColumnName(column, columns, headerRowIndex) { | |||
| export function changeColumnHeader(column, columns, headerRowIndex, mergeDuplicateHeaders, newColumnName) { | |||
| let cloneColumn = R.mergeRight({}, column); | |||
There was a problem hiding this comment.
There's no need to clone the column if it's not modified in the if below. R.set below (https://github.com/plotly/dash-table/pull/498/files#diff-4102a065e79334d16000051ec35189f7R75) will treat the entire data structure as immutable in any case.
src/dash-table/utils/actions.js
Outdated
| } | ||
| const transformedColumns = columns.slice(0); | ||
| const columnIndex = columns.findIndex(col => col.id === column.id); | ||
| transformedColumns[columnIndex] = cloneColumn; |
There was a problem hiding this comment.
Similarly, because of how R.set works, it's unnecessary to clone columns if the affected column above is not modified.
Fixes #491