From 82ae587e9e11286ff6129e06f71ec3cc2304223b Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Thu, 2 Jan 2020 12:55:35 -0700 Subject: [PATCH 1/4] Fix bug in datagrid where columns in Hide fields were cached; added useResettingState --- src/components/datagrid/column_selector.tsx | 6 +- src/components/datagrid/data_grid.test.tsx | 327 +++++++++++++----- src/services/hooks/index.ts | 1 + src/services/hooks/useResettingState.test.tsx | 35 ++ src/services/hooks/useResettingState.ts | 23 ++ src/services/index.ts | 2 + 6 files changed, 311 insertions(+), 83 deletions(-) create mode 100644 src/services/hooks/index.ts create mode 100644 src/services/hooks/useResettingState.test.tsx create mode 100644 src/services/hooks/useResettingState.ts diff --git a/src/components/datagrid/column_selector.tsx b/src/components/datagrid/column_selector.tsx index e4cad45b2bf..6dba51bb52f 100644 --- a/src/components/datagrid/column_selector.tsx +++ b/src/components/datagrid/column_selector.tsx @@ -27,13 +27,15 @@ import { } from '../drag_and_drop'; import { DropResult } from 'react-beautiful-dnd'; import { EuiIcon } from '../icon'; +import { useResettingState } from '../../services'; export const useColumnSelector = ( availableColumns: EuiDataGridColumn[], columnVisibility: EuiDataGridColumnVisibility ): [ReactElement, EuiDataGridColumn[]] => { - const [sortedColumns, setSortedColumns] = useState(() => - availableColumns.map(({ id }) => id) + const [sortedColumns, setSortedColumns] = useResettingState( + () => availableColumns.map(({ id }) => id), + [availableColumns] ); const { visibleColumns, setVisibleColumns } = columnVisibility; diff --git a/src/components/datagrid/data_grid.test.tsx b/src/components/datagrid/data_grid.test.tsx index cd4f577da19..34d49779f44 100644 --- a/src/components/datagrid/data_grid.test.tsx +++ b/src/components/datagrid/data_grid.test.tsx @@ -74,23 +74,37 @@ function resizeColumn( datagrid.update(); } -function getColumnSortDirection( - datagrid: ReactWrapper, - columnId: string -): [ReactWrapper, string] { - // get the button that sorts by this column - let columnSorter = datagrid.find( - `div[data-test-subj="euiDataGridColumnSorting-sortColumn-${columnId}"]` +function openColumnSorterSelection(datagrid: ReactWrapper) { + let columnSelectionPopover = datagrid.find( + 'EuiPopover[data-test-subj="dataGridColumnSortingPopoverColumnSelection"]' ); - if (columnSorter.length === 0) { - // need to enable this column + expect(columnSelectionPopover).not.euiPopoverToBeOpen(); + const popoverButton = columnSelectionPopover + .find('div[className="euiPopover__anchor"]') + .find('[onClick]') + .first(); + // @ts-ignore-next-line + act(() => popoverButton.props().onClick()); - // open the column selection popover - let columnSelectionPopover = datagrid.find( - 'EuiPopover[data-test-subj="dataGridColumnSortingPopoverColumnSelection"]' - ); - expect(columnSelectionPopover).not.euiPopoverToBeOpen(); - let popoverButton = columnSelectionPopover + datagrid.update(); + + columnSelectionPopover = datagrid.find( + 'EuiPopover[data-test-subj="dataGridColumnSortingPopoverColumnSelection"]' + ); + expect(columnSelectionPopover).euiPopoverToBeOpen(); + + return columnSelectionPopover; +} + +function closeColumnSorterSelection(datagrid: ReactWrapper) { + let columnSelectionPopover = datagrid.find( + 'EuiPopover[data-test-subj="dataGridColumnSortingPopoverColumnSelection"]' + ); + // popover will go away if all of the columns are selected + if (columnSelectionPopover.length > 0) { + expect(columnSelectionPopover).euiPopoverToBeOpen(); + + const popoverButton = columnSelectionPopover .find('div[className="euiPopover__anchor"]') .find('[onClick]') .first(); @@ -102,7 +116,23 @@ function getColumnSortDirection( columnSelectionPopover = datagrid.find( 'EuiPopover[data-test-subj="dataGridColumnSortingPopoverColumnSelection"]' ); - expect(columnSelectionPopover).euiPopoverToBeOpen(); + expect(columnSelectionPopover).not.euiPopoverToBeOpen(); + } + + return columnSelectionPopover; +} + +function getColumnSortDirection( + datagrid: ReactWrapper, + columnId: string +): [ReactWrapper, string] { + // get the button that sorts by this column + let columnSorter = datagrid.find( + `div[data-test-subj="euiDataGridColumnSorting-sortColumn-${columnId}"]` + ); + if (columnSorter.length === 0) { + // need to enable this column + openColumnSorterSelection(datagrid); // find button to enable this column and click it const selectColumnButton = datagrid.find( @@ -113,30 +143,10 @@ function getColumnSortDirection( act(() => selectColumnButton.props().onClick()); // close column selection popover - columnSelectionPopover = datagrid.find( - 'EuiPopover[data-test-subj="dataGridColumnSortingPopoverColumnSelection"]' - ); - // popover will go away if all of the columns are selected - if (columnSelectionPopover.length > 0) { - expect(columnSelectionPopover).euiPopoverToBeOpen(); - - popoverButton = columnSelectionPopover - .find('div[className="euiPopover__anchor"]') - .find('[onClick]') - .first(); - // @ts-ignore-next-line - act(() => popoverButton.props().onClick()); - - datagrid.update(); - - columnSelectionPopover = datagrid.find( - 'EuiPopover[data-test-subj="dataGridColumnSortingPopoverColumnSelection"]' - ); - expect(columnSelectionPopover).not.euiPopoverToBeOpen(); - } + closeColumnSorterSelection(datagrid); // find the column sorter - columnSelectionPopover = datagrid.find( + const columnSelectionPopover = datagrid.find( 'EuiPopover[data-test-subj="dataGridColumnSortingPopover"]' ); columnSorter = columnSelectionPopover.find( @@ -157,18 +167,13 @@ function getColumnSortDirection( return [columnSorter, sortDirection]; } -function sortByColumn( - datagrid: ReactWrapper, - columnId: string, - direction: 'asc' | 'desc' | 'off' -) { - // open datagrid sorting options +function openColumnSorter(datagrid: ReactWrapper) { let popover = datagrid.find( 'EuiPopover[data-test-subj="dataGridColumnSortingPopover"]' ); expect(popover).not.euiPopoverToBeOpen(); - let popoverButton = popover + const popoverButton = popover .find('div[className="euiPopover__anchor"]') .find('[onClick]') .first(); @@ -182,6 +187,39 @@ function sortByColumn( ); expect(popover).euiPopoverToBeOpen(); + return popover; +} + +function closeColumnSorter(datagrid: ReactWrapper) { + let popover = datagrid.find( + 'EuiPopover[data-test-subj="dataGridColumnSortingPopover"]' + ); + expect(popover).euiPopoverToBeOpen(); + + const popoverButton = popover + .find('div[className="euiPopover__anchor"]') + .find('[onClick]') + .first(); + // @ts-ignore-next-line + act(() => popoverButton.props().onClick()); + + datagrid.update(); + + popover = datagrid.find( + 'EuiPopover[data-test-subj="dataGridColumnSortingPopover"]' + ); + expect(popover).not.euiPopoverToBeOpen(); + + return popover; +} + +function sortByColumn( + datagrid: ReactWrapper, + columnId: string, + direction: 'asc' | 'desc' | 'off' +) { + openColumnSorter(datagrid); + let [columnSorter, currentSortDirection] = getColumnSortDirection( datagrid, columnId @@ -214,25 +252,7 @@ function sortByColumn( sortButton.simulate('change', [undefined, direction]); } - // close popover - popover = datagrid.find( - 'EuiPopover[data-test-subj="dataGridColumnSortingPopover"]' - ); - expect(popover).euiPopoverToBeOpen(); - - popoverButton = popover - .find('div[className="euiPopover__anchor"]') - .find('[onClick]') - .first(); - // @ts-ignore-next-line - act(() => popoverButton.props().onClick()); - - datagrid.update(); - - popover = datagrid.find( - 'EuiPopover[data-test-subj="dataGridColumnSortingPopover"]' - ); - expect(popover).not.euiPopoverToBeOpen(); + closeColumnSorter(datagrid); } expect.extend({ @@ -278,18 +298,13 @@ declare global { } } -function setColumnVisibility( - datagrid: ReactWrapper, - columnId: string, - isVisible: boolean -) { - // open datagrid column options +function openColumnSelector(datagrid: ReactWrapper) { let popover = datagrid.find( 'EuiPopover[data-test-subj="dataGridColumnSelectorPopover"]' ); expect(popover).not.euiPopoverToBeOpen(); - let popoverButton = popover + const popoverButton = popover .find('div[className="euiPopover__anchor"]') .find('[onClick]') .first(); @@ -303,21 +318,16 @@ function setColumnVisibility( ); expect(popover).euiPopoverToBeOpen(); - // toggle column's visibility switch - const portal = popover.find('EuiPortal'); - - const columnSwitch = portal.find(`EuiSwitch[name="${columnId}"]`); - const switchInput = columnSwitch.find('button'); - switchInput.getDOMNode().setAttribute('aria-checked', `${isVisible}`); - switchInput.simulate('click'); + return popover; +} - // close popover - popover = datagrid.find( +function closeColumnSelector(datagrid: ReactWrapper) { + let popover = datagrid.find( 'EuiPopover[data-test-subj="dataGridColumnSelectorPopover"]' ); expect(popover).euiPopoverToBeOpen(); - popoverButton = popover + const popoverButton = popover .find('div[className="euiPopover__anchor"]') .find('[onClick]') .first(); @@ -330,6 +340,26 @@ function setColumnVisibility( 'EuiPopover[data-test-subj="dataGridColumnSelectorPopover"]' ); expect(popover).not.euiPopoverToBeOpen(); + + return popover; +} + +function setColumnVisibility( + datagrid: ReactWrapper, + columnId: string, + isVisible: boolean +) { + const popover = openColumnSelector(datagrid); + + // toggle column's visibility switch + const portal = popover.find('EuiPortal'); + + const columnSwitch = portal.find(`EuiSwitch[name="${columnId}"]`); + const switchInput = columnSwitch.find('button'); + switchInput.getDOMNode().setAttribute('aria-checked', `${isVisible}`); + switchInput.simulate('click'); + + closeColumnSelector(datagrid); } function moveColumnToIndex( @@ -1558,6 +1588,141 @@ Array [ }); }); + describe('updating column definitions', () => { + it('renders the new set', () => { + const component = mount( + {}, + }} + rowCount={2} + renderCellValue={({ rowIndex, columnId }) => + `${rowIndex}-${columnId}` + } + /> + ); + + expect(extractGridData(component)).toEqual([ + ['A', 'B'], + ['0-A', '0-B'], + ['1-A', '1-B'], + ]); + + component.setProps({ + columns: [{ id: 'A' }, { id: 'C' }], + columnVisibility: { + visibleColumns: ['A', 'C'], + setVisibleColumns: () => {}, + }, + }); + + expect(extractGridData(component)).toEqual([ + ['A', 'C'], + ['0-A', '0-C'], + ['1-A', '1-C'], + ]); + }); + + it('"Hide fields" updates', () => { + const component = mount( + {}, + }} + rowCount={2} + renderCellValue={({ rowIndex, columnId }) => + `${rowIndex}-${columnId}` + } + /> + ); + + // verify original column list is A, B + let popover = openColumnSelector(component); + expect( + popover + .find('.euiDataGridColumnSelector__item') + .map(item => item.text()) + ).toEqual(['A', 'B']); + closeColumnSelector(component); + + // update columns + component.setProps({ + columns: [{ id: 'A' }, { id: 'C' }], + columnVisibility: { + visibleColumns: ['A', 'C'], + setVisibleColumns: () => {}, + }, + }); + + // test that the column list updated to A,C + popover = openColumnSelector(component); + expect( + popover + .find('.euiDataGridColumnSelector__item') + .map(item => item.text()) + ).toEqual(['A', 'C']); + closeColumnSelector(component); + }); + + it('"Sort fields" updates', () => { + const component = mount( + {}, + }} + sorting={{ + onSort: () => {}, + columns: [], + }} + rowCount={2} + renderCellValue={({ rowIndex, columnId }) => + `${rowIndex}-${columnId}` + } + /> + ); + + // verify original column list is A, B + openColumnSorter(component); + let popover = openColumnSorterSelection(component); + expect( + popover + .find('.euiDataGridColumnSorting__field') + .map(item => item.text()) + ).toEqual(['A', 'B']); + closeColumnSorterSelection(component); + closeColumnSorter(component); + + // update columns + component.setProps({ + columns: [{ id: 'A' }, { id: 'C' }], + columnVisibility: { + visibleColumns: ['A', 'C'], + setVisibleColumns: () => {}, + }, + }); + + // test that the column list updated to A,C + openColumnSorter(component); + popover = openColumnSorterSelection(component); + expect( + popover + .find('.euiDataGridColumnSorting__field') + .map(item => item.text()) + ).toEqual(['A', 'C']); + closeColumnSorterSelection(component); + closeColumnSorter(component); + }); + }); + describe('keyboard controls', () => { it('supports simple arrow navigation', () => { let pagination = { diff --git a/src/services/hooks/index.ts b/src/services/hooks/index.ts new file mode 100644 index 00000000000..ee248bb3ff9 --- /dev/null +++ b/src/services/hooks/index.ts @@ -0,0 +1 @@ +export * from './useResettingState'; diff --git a/src/services/hooks/useResettingState.test.tsx b/src/services/hooks/useResettingState.test.tsx new file mode 100644 index 00000000000..f0e756a164c --- /dev/null +++ b/src/services/hooks/useResettingState.test.tsx @@ -0,0 +1,35 @@ +import React from 'react'; +import { mount } from 'enzyme'; +import { useResettingState } from './useResettingState'; + +describe('useResettingState', () => { + it('sets the base state', () => { + // this is a huge abuse of closure scope + // but allows for jest's built in mock expect'ing + let sourceValue = 2; + const doubler = jest.fn(() => { + return sourceValue * 2; + }); + + function Foo() { + const [value] = useResettingState(doubler, [sourceValue]); + + return
{value}
; + } + + // mount the component verify the state function was called with no previous state value + const component = mount(); + expect(doubler).toHaveBeenCalledTimes(1); + expect(doubler).toHaveBeenCalledWith(); + expect(component.text()).toBe('4'); // 2 * 2 + + doubler.mockClear(); + + // update the source value, force a re-render, and run checks + sourceValue = 4; + component.setProps({}); + expect(doubler).toHaveBeenCalledTimes(1); + expect(doubler).toHaveBeenCalledWith(4); // check previous state value + expect(component.text()).toBe('8'); // new value should be 4 * 2 + }); +}); diff --git a/src/services/hooks/useResettingState.ts b/src/services/hooks/useResettingState.ts new file mode 100644 index 00000000000..694acf636c4 --- /dev/null +++ b/src/services/hooks/useResettingState.ts @@ -0,0 +1,23 @@ +import { useEffect, useState } from 'react'; + +export function useResettingState( + valueFn: (previousState: undefined | T) => T, + deps: unknown[] +) { + const [state, setState] = useState(valueFn as () => T); + const [updateCount, setUpdateCount] = useState(0); + + useEffect(() => { + // don't call setState on initial mount + if (updateCount > 0) { + setState(valueFn); + } + setUpdateCount(updateCount => ++updateCount); + + // purposefully omitting `updateCount`, `setUpdateCount`, and `valueFn` + // this means updating only the valueFn has no effect, but allows for more natural feeling hook use + // eslint-disable-next-line react-hooks/exhaustive-deps + }, deps); + + return [state, setState] as const; +} diff --git a/src/services/index.ts b/src/services/index.ts index 96ae2bc8296..470692520de 100644 --- a/src/services/index.ts +++ b/src/services/index.ts @@ -74,3 +74,5 @@ export { } from './transition'; export { EuiWindowEvent } from './window_event'; + +export { useResettingState } from './hooks'; From 80739458bd5718e596316215a84ff46ab7d88271 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Thu, 2 Jan 2020 13:04:37 -0700 Subject: [PATCH 2/4] changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c52426ad960..ccce595b902 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,10 +5,12 @@ - Added `tableLayout` prop to `EuiTable`, `EuiBasicTable` and `EuiInMemoryTable` to provide the option of auto layout ([#2697](https://github.com/elastic/eui/pull/2697)) - Converted `EuiSuggest` to Typescript ([#2692](https://github.com/elastic/eui/pull/2692)) - Converted `EuiErrorBoundary` to Typescript ([#2690](https://github.com/elastic/eui/pull/2690)) +- Added `useResettingState` custom hook ([#2725](https://github.com/elastic/eui/pull/#2725)) **Bug fixes** - Fixed `isExpanded` property of nodes from `EuiTreeView` ([#2700](https://github.com/elastic/eui/pull/#2700)) +- Fixed bug in `EuiDataGrid` that prevented the "Hide fields" popover from showing an updated column list ([#2725](https://github.com/elastic/eui/pull/#2725)) ## [`17.3.1`](https://github.com/elastic/eui/tree/v17.3.1) From e7ccdb00bb1cb44250f62306386e7276108c7a4f Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Thu, 2 Jan 2020 14:26:35 -0700 Subject: [PATCH 3/4] Move type declaration to a jest-friendly place --- src/services/hooks/useResettingState.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/services/hooks/useResettingState.ts b/src/services/hooks/useResettingState.ts index 694acf636c4..bbadb0ac128 100644 --- a/src/services/hooks/useResettingState.ts +++ b/src/services/hooks/useResettingState.ts @@ -3,7 +3,7 @@ import { useEffect, useState } from 'react'; export function useResettingState( valueFn: (previousState: undefined | T) => T, deps: unknown[] -) { +): [T, React.Dispatch>] { const [state, setState] = useState(valueFn as () => T); const [updateCount, setUpdateCount] = useState(0); @@ -19,5 +19,5 @@ export function useResettingState( // eslint-disable-next-line react-hooks/exhaustive-deps }, deps); - return [state, setState] as const; + return [state, setState]; } From bad14c7f7290eecd506be794e76264fe7b0833fa Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Fri, 3 Jan 2020 10:10:04 -0700 Subject: [PATCH 4/4] Renamed useResettingState -> useDependentState, swapped a useState for a useRef --- CHANGELOG.md | 2 +- src/components/datagrid/column_selector.tsx | 4 ++-- src/services/hooks/index.ts | 2 +- ...gState.test.tsx => useDependentState.test.tsx} | 6 +++--- ...{useResettingState.ts => useDependentState.ts} | 15 +++++++++------ src/services/index.ts | 2 +- 6 files changed, 17 insertions(+), 14 deletions(-) rename src/services/hooks/{useResettingState.test.tsx => useDependentState.test.tsx} (85%) rename src/services/hooks/{useResettingState.ts => useDependentState.ts} (57%) diff --git a/CHANGELOG.md b/CHANGELOG.md index a69e3a0c6e2..283a7ba07a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ - Updated `EuiNavDrawer` to accept React fragments ([#2710](https://github.com/elastic/eui/pull/2710)) - Added `EuiFormFieldset` and `EuiFormLegend` components ([#2706](https://github.com/elastic/eui/pull/2706)) - Adjusted colors of color blind viz palette ([#2686](https://github.com/elastic/eui/pull/2686)) -- Added `useResettingState` custom hook ([#2725](https://github.com/elastic/eui/pull/#2725)) +- Added `useDependentState` custom hook ([#2725](https://github.com/elastic/eui/pull/#2725)) **Bug fixes** diff --git a/src/components/datagrid/column_selector.tsx b/src/components/datagrid/column_selector.tsx index 6dba51bb52f..c92882f7e41 100644 --- a/src/components/datagrid/column_selector.tsx +++ b/src/components/datagrid/column_selector.tsx @@ -27,13 +27,13 @@ import { } from '../drag_and_drop'; import { DropResult } from 'react-beautiful-dnd'; import { EuiIcon } from '../icon'; -import { useResettingState } from '../../services'; +import { useDependentState } from '../../services'; export const useColumnSelector = ( availableColumns: EuiDataGridColumn[], columnVisibility: EuiDataGridColumnVisibility ): [ReactElement, EuiDataGridColumn[]] => { - const [sortedColumns, setSortedColumns] = useResettingState( + const [sortedColumns, setSortedColumns] = useDependentState( () => availableColumns.map(({ id }) => id), [availableColumns] ); diff --git a/src/services/hooks/index.ts b/src/services/hooks/index.ts index ee248bb3ff9..27b86bf36fb 100644 --- a/src/services/hooks/index.ts +++ b/src/services/hooks/index.ts @@ -1 +1 @@ -export * from './useResettingState'; +export * from './useDependentState'; diff --git a/src/services/hooks/useResettingState.test.tsx b/src/services/hooks/useDependentState.test.tsx similarity index 85% rename from src/services/hooks/useResettingState.test.tsx rename to src/services/hooks/useDependentState.test.tsx index f0e756a164c..49346bdc2d6 100644 --- a/src/services/hooks/useResettingState.test.tsx +++ b/src/services/hooks/useDependentState.test.tsx @@ -1,8 +1,8 @@ import React from 'react'; import { mount } from 'enzyme'; -import { useResettingState } from './useResettingState'; +import { useDependentState } from './useDependentState'; -describe('useResettingState', () => { +describe('useDependentState', () => { it('sets the base state', () => { // this is a huge abuse of closure scope // but allows for jest's built in mock expect'ing @@ -12,7 +12,7 @@ describe('useResettingState', () => { }); function Foo() { - const [value] = useResettingState(doubler, [sourceValue]); + const [value] = useDependentState(doubler, [sourceValue]); return
{value}
; } diff --git a/src/services/hooks/useResettingState.ts b/src/services/hooks/useDependentState.ts similarity index 57% rename from src/services/hooks/useResettingState.ts rename to src/services/hooks/useDependentState.ts index bbadb0ac128..05b24715127 100644 --- a/src/services/hooks/useResettingState.ts +++ b/src/services/hooks/useDependentState.ts @@ -1,20 +1,23 @@ -import { useEffect, useState } from 'react'; +import { useEffect, useState, useRef } from 'react'; -export function useResettingState( +export function useDependentState( valueFn: (previousState: undefined | T) => T, deps: unknown[] ): [T, React.Dispatch>] { const [state, setState] = useState(valueFn as () => T); - const [updateCount, setUpdateCount] = useState(0); + + // use ref instead of a state to avoid causing an unnecessary re-render + const hasMounted = useRef(false); useEffect(() => { // don't call setState on initial mount - if (updateCount > 0) { + if (hasMounted.current === true) { setState(valueFn); + } else { + hasMounted.current = true; } - setUpdateCount(updateCount => ++updateCount); - // purposefully omitting `updateCount`, `setUpdateCount`, and `valueFn` + // purposefully omitting `updateCount.current` and `valueFn` // this means updating only the valueFn has no effect, but allows for more natural feeling hook use // eslint-disable-next-line react-hooks/exhaustive-deps }, deps); diff --git a/src/services/index.ts b/src/services/index.ts index 067c1180343..4ab21066f31 100644 --- a/src/services/index.ts +++ b/src/services/index.ts @@ -85,4 +85,4 @@ export { export { EuiWindowEvent } from './window_event'; -export { useResettingState } from './hooks'; +export { useDependentState } from './hooks';