diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c04c1bd5..75ac544ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ This project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] ### Fixed +[#578](https://github.com/plotly/dash-table/pull/578) +- Fix [#576](https://github.com/plotly/dash-table/issues/576), editing column names or deleting columns while other columns are hidden causing the hidden columns to be lost. +- Fix an unreported bug that clicking "Cancel" at the column name edit prompt would clear the name, rather than leaving it unchanged as it should. + [#569](https://github.com/plotly/dash-table/issues/569), [#544](https://github.com/plotly/dash-table/issues/544) - Allow empty strings in all `filter_query` (e.g filter_query: '{colA} eq ""') diff --git a/src/dash-table/derived/header/content.tsx b/src/dash-table/derived/header/content.tsx index a78fccf57..dfe806260 100644 --- a/src/dash-table/derived/header/content.tsx +++ b/src/dash-table/derived/header/content.tsx @@ -26,6 +26,7 @@ const doAction = ( action: ( column: IColumn, columns: Columns, + visibleColumns: Columns, columnRowIndex: any, mergeDuplicateHeaders: boolean, data: Data @@ -33,6 +34,7 @@ const doAction = ( selected_columns: string[], column: IColumn, columns: Columns, + visibleColumns: Columns, columnRowIndex: any, mergeDuplicateHeaders: boolean, setFilter: SetFilter, @@ -40,7 +42,7 @@ const doAction = ( map: Map, data: Data ) => () => { - const props = action(column, columns, columnRowIndex, mergeDuplicateHeaders, data); + const props = action(column, columns, visibleColumns, columnRowIndex, mergeDuplicateHeaders, data); const affectedColumIds = actions.getAffectedColumns(column, columns, columnRowIndex, mergeDuplicateHeaders); @@ -99,7 +101,10 @@ function doSort(columnId: ColumnId, sortBy: SortBy, mode: SortMode, setProps: Se function editColumnName(column: IColumn, columns: Columns, columnRowIndex: any, setProps: SetProps, mergeDuplicateHeaders: boolean) { return () => { - setProps(actions.editColumnName(column, columns, columnRowIndex, mergeDuplicateHeaders)); + const update = actions.editColumnName(column, columns, columnRowIndex, mergeDuplicateHeaders); + if (update) { + setProps(update); + } }; } @@ -250,7 +255,7 @@ function getter( null : ( ) @@ -260,7 +265,7 @@ function getter( null : ( ) @@ -272,7 +277,7 @@ function getter( className={'column-header--delete' + (spansAllColumns ? ' disabled' : '')} onClick={spansAllColumns ? undefined : - doAction(actions.deleteColumn, selected_columns, column, visibleColumns, headerRowIndex, mergeDuplicateHeaders, setFilter, setProps, map, data) + doAction(actions.deleteColumn, selected_columns, column, columns, visibleColumns, headerRowIndex, mergeDuplicateHeaders, setFilter, setProps, map, data) } > diff --git a/src/dash-table/utils/actions.js b/src/dash-table/utils/actions.js index c11ccab7d..5bf9f9e08 100644 --- a/src/dash-table/utils/actions.js +++ b/src/dash-table/utils/actions.js @@ -47,26 +47,25 @@ export function getAffectedColumns(column, columns, headerRowIndex, mergeDuplica ); } -export function clearColumn(column, columns, headerRowIndex, mergeDuplicateHeaders, data) { - const rejectedColumnIds = getAffectedColumns(column, columns, headerRowIndex, mergeDuplicateHeaders); - - return { - data: R.map(R.omit(rejectedColumnIds), data) - }; +export function clearColumn(column, columns, visibleColumns, headerRowIndex, mergeDuplicateHeaders, _data) { + const {data} = deleteColumn(column, columns, visibleColumns, headerRowIndex, mergeDuplicateHeaders, _data); + return {data}; } -export function deleteColumn(column, columns, headerRowIndex, mergeDuplicateHeaders, data) { - const {groupIndexFirst, groupIndexLast} = getGroupedColumnIndices( - column, columns, headerRowIndex, mergeDuplicateHeaders, columns.indexOf(column) +export function deleteColumn(column, columns, visibleColumns, headerRowIndex, mergeDuplicateHeaders, data) { + const rejectedColumnIds = getAffectedColumns( + column, + visibleColumns, + headerRowIndex, + mergeDuplicateHeaders ); return { - columns: R.remove( - groupIndexFirst, - 1 + groupIndexLast - groupIndexFirst, + columns: R.filter( + col => (rejectedColumnIds.indexOf(col.id) === -1), columns ), - ...clearColumn(column, columns, headerRowIndex, mergeDuplicateHeaders, data), + data: R.map(R.omit(rejectedColumnIds), data), // NOTE - We're just clearing these so that there aren't any // inconsistencies. In an ideal world, we would probably only // update them if they contained one of the columns that we're @@ -106,7 +105,7 @@ export function changeColumnHeader(column, columns, headerRowIndex, mergeDuplica } const { groupIndexFirst, groupIndexLast } = getGroupedColumnIndices( - column, newColumns, headerRowIndex, mergeDuplicateHeaders, columnIndex + column, newColumns, headerRowIndex, mergeDuplicateHeaders, columnIndex, true ); R.range(groupIndexFirst, groupIndexLast + 1).map(i => { @@ -122,5 +121,8 @@ export function changeColumnHeader(column, columns, headerRowIndex, mergeDuplica export function editColumnName(column, columns, headerRowIndex, mergeDuplicateHeaders) { const newColumnName = window.prompt('Enter a new column name'); + if (newColumnName === null) { + return null; + } return changeColumnHeader(column, columns, headerRowIndex, mergeDuplicateHeaders, newColumnName); } diff --git a/tests/cypress/tests/standalone/column_test.ts b/tests/cypress/tests/standalone/column_test.ts index fec1b80a1..10664a49a 100644 --- a/tests/cypress/tests/standalone/column_test.ts +++ b/tests/cypress/tests/standalone/column_test.ts @@ -27,6 +27,17 @@ describe(`column, mode=${AppMode.Actionable}, flavor=${AppFlavor.Merged}, ${AppF DashTable.getHeader(1, 1).within(() => cy.get('span.column-header-name').should('have.html', 'Paris')); }); + it('keeps hidden pieces when deleting a merged column', () => { + cy.get('.column-4 .column-header--hide').click(); // Boston + cy.get('.column-2 .column-header--hide').click(); // Montréal + DashTable.deleteColumnById(0, 'ccc'); // City + cy.get('.show-hide').click(); + cy.get('.show-hide-menu-item input').eq(1).click(); // Montréal + cy.get('.show-hide-menu-item input').eq(2).click(); // Boston + cy.get('.column-1 .column-header-name').eq(2).should('have.html', 'Montréal'); + cy.get('.column-2 .column-header-name').eq(1).should('have.html', 'Boston'); + }); + it('can clear column', () => { DashTable.getFilter(0).click(); DOM.focused.type(`is num`); @@ -62,6 +73,29 @@ describe(`column, mode=${AppMode.Actionable}, flavor=${AppFlavor.Merged}, ${AppF DashTable.getFilter(4).within(() => cy.get('input').should('have.value', '')); }); + it('keeps hidden pieces when clearing a merged column', () => { + // initial state of city columns + DashTable.getCell(0, 1).within(() => cy.get('.dash-cell-value').should('have.html', '1')); + DashTable.getCell(0, 2).within(() => cy.get('.dash-cell-value').should('have.html', '100')); + DashTable.getCell(0, 3).within(() => cy.get('.dash-cell-value').should('have.html', '1')); + DashTable.getCell(0, 4).within(() => cy.get('.dash-cell-value').should('have.html', '2')); + DashTable.getCell(0, 5).within(() => cy.get('.dash-cell-value').should('have.html', '10')); + + cy.get('.column-4 .column-header--hide').click(); // Boston + cy.get('.column-2 .column-header--hide').click(); // Montréal + DashTable.clearColumnById(0, 'ccc'); // City + cy.get('.show-hide').click(); + cy.get('.show-hide-menu-item input').eq(2).click(); // Montréal + cy.get('.show-hide-menu-item input').eq(4).click(); // Boston + + // after clear and re-show -> only Montréal and Boston have data + DashTable.getCell(0, 1).within(() => cy.get('.dash-cell-value').should('have.html', '')); + DashTable.getCell(0, 2).within(() => cy.get('.dash-cell-value').should('have.html', '100')); + DashTable.getCell(0, 3).within(() => cy.get('.dash-cell-value').should('have.html', '')); + DashTable.getCell(0, 4).within(() => cy.get('.dash-cell-value').should('have.html', '2')); + DashTable.getCell(0, 5).within(() => cy.get('.dash-cell-value').should('have.html', '')); + }); + it('can hide column', () => { DashTable.getHeader(0, 0).within(() => cy.get('span.column-header-name').should('have.html', 'rows')); DashTable.hideColumnById(0, 'rows'); @@ -313,4 +347,4 @@ describe(`column, mode=${AppMode.Actionable}`, () => { DashTable.getHeader(1, 2).within(() => cy.get('span.column-header-name').should('have.html', 'France')); DashTable.getHeader(2, 2).within(() => cy.get('span.column-header-name').should('have.html', 'Paris')); }); -}); \ No newline at end of file +}); diff --git a/tests/cypress/tests/standalone/edit_headers.ts b/tests/cypress/tests/standalone/edit_headers.ts index 746e05e00..c24d02e06 100644 --- a/tests/cypress/tests/standalone/edit_headers.ts +++ b/tests/cypress/tests/standalone/edit_headers.ts @@ -51,6 +51,49 @@ describe(`edit, mode=${AppMode.Typed}`, () => { }); }); + describe(`edit headers while some columns are hidden`, () => { + beforeEach(() => { + cy.visit(`http://localhost:8080?mode=${AppMode.Actionable}&flavor=${AppFlavor.Merged}`); + DashTable.toggleScroll(false); + }) + it('preserves hidden columns unchanged when editing visible names', () => { + cy.window().then((win: any) => { + cy.stub(win, 'prompt').returns('Chill'); + }); + // hide 3 columns - one at the start of a merged set, one in the middle, one not in the set + cy.get('.column-8 .column-header--hide').click({force: true}); + cy.get('.column-6 .column-header--hide').click({force: true}); + cy.get('.column-1 .column-header--hide').click({force: true}); + // edit the merged name + cy.get('.dash-header.column-5 .column-header--edit').eq(1).click({force: true}); + // re-show the hidden columns + cy.get('.show-hide').click(); + cy.get('.show-hide-menu-item input').eq(1).click(); + cy.get('.show-hide-menu-item input').eq(6).click(); + cy.get('.show-hide-menu-item input').eq(8).click(); + // all columns still exist + cy.get('.dash-header.column-9 .column-header-name').should('have.html', 'Temperature-RO'); + // the columns that were hidden when the merged name was changed + // still changed name - so they're still all merged + cy.get('.dash-header.column-6[colspan="4"] .column-header-name').eq(1).should('have.html', 'Chill'); + }); + }); + + describe('edit headers, canceled edit', () => { + beforeEach(() => { + cy.visit(`http://localhost:8080?mode=${AppMode.SingleHeaders}`); + DashTable.toggleScroll(false); + }); + it('preserves column name if edit is canceled', () => { + cy.window().then((win: any) => { + // user presses cancel -> prompt returns null + cy.stub(win, 'prompt').returns(null); + }); + cy.get('.dash-header.column-0 .column-header--edit').eq(0).click(); + cy.get('.dash-header.column-0 > div > span:last-child').should('have.html', 'rows'); + }); + }); + describe(`edit headers, mode=${AppMode.SingleHeaders}`, () => { beforeEach(() => { cy.visit(`http://localhost:8080?mode=${AppMode.SingleHeaders}`);