Merge pull request #578 from plotly/576-hidden-edits

alexcjohnson · web-flow · commit 293ee3355e61 · 2019-09-13T21:58:37.000-04:00
576 - column edits with hidden columns
diff --git 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
@@ -26,21 +26,23 @@ const doAction = (
     action: (
         column: IColumn,
         columns: Columns,
+        visibleColumns: Columns,
         columnRowIndex: any,
         mergeDuplicateHeaders: boolean,
         data: Data
     ) => any,
     selected_columns: string[],
     column: IColumn,
     columns: Columns,
+    visibleColumns: Columns,
     columnRowIndex: any,
     mergeDuplicateHeaders: boolean,
     setFilter: SetFilter,
     setProps: SetProps,
     map: Map<string, SingleColumnSyntaxTree>,
     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 :
                             (<span
                                 className='column-header--edit'
-                                onClick={editColumnName(column, visibleColumns, headerRowIndex, setProps, mergeDuplicateHeaders)}
+                                onClick={editColumnName(column, columns, headerRowIndex, setProps, mergeDuplicateHeaders)}
                             >
                                 <FontAwesomeIcon icon='pencil-alt' />
                             </span>)
@@ -260,7 +265,7 @@ function getter(
                             null :
                             (<span
                                 className='column-header--clear'
-                                onClick={doAction(actions.clearColumn, selected_columns, column, visibleColumns, headerRowIndex, mergeDuplicateHeaders, setFilter, setProps, map, data)}
+                                onClick={doAction(actions.clearColumn, selected_columns, column, columns, visibleColumns, headerRowIndex, mergeDuplicateHeaders, setFilter, setProps, map, data)}
                             >
                                 <FontAwesomeIcon icon='eraser' />
                             </span>)
@@ -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)
                                 }
                             >
                                 <FontAwesomeIcon icon={['far', 'trash-alt']} />
diff --git 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
@@ -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'));
     });
-});
+});
diff --git 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}`);
