From 89b27831af80d2f104cdcadf05075e854ddbafed Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Wed, 10 Nov 2021 14:48:51 -0800 Subject: [PATCH] [Single UI polish] Set dynamic minRowHeight based on grid density Previously rows had a static 34px min height which looked super out of place next to lineCount (which correctly sets row height based on density). This especially looked bad after the align-items baseline change (which was required to get multi-line elements working however). This basically updates the single/undefined state to match how lineCount work and to dynamically update the row height based on density. It also increase default row height for normal density to 36px - assuming Amsterdam theme as default. I was previously using '34' as a shorthand for 'default height' in many unit tests, so I've done a general find&replace where it made sense I also removed the default row height fallback for rowHeightUtils - getCalculatedHeight will pick the defaultHeight (which is minRowHeight) in any case since it will be larger than 0, so there's no need for this to be a realistic number --- .../__snapshots__/data_grid.test.tsx.snap | 68 +++++++++---------- .../data_grid_body.test.tsx.snap | 6 +- .../datagrid/body/data_grid_body.test.tsx | 3 +- .../datagrid/body/data_grid_body.tsx | 8 +-- .../controls/display_selector.test.tsx | 44 +++++++++++- .../datagrid/controls/display_selector.tsx | 13 +++- src/components/datagrid/data_grid.test.tsx | 22 +++--- src/components/datagrid/data_grid.tsx | 2 + src/components/datagrid/data_grid_types.ts | 1 + .../datagrid/row_height_utils.test.ts | 40 +++++------ src/components/datagrid/row_height_utils.ts | 3 +- 11 files changed, 132 insertions(+), 78 deletions(-) diff --git a/src/components/datagrid/__snapshots__/data_grid.test.tsx.snap b/src/components/datagrid/__snapshots__/data_grid.test.tsx.snap index e7b8a263bb5e..72d3af414168 100644 --- a/src/components/datagrid/__snapshots__/data_grid.test.tsx.snap +++ b/src/components/datagrid/__snapshots__/data_grid.test.tsx.snap @@ -1047,7 +1047,7 @@ Array [ style="position:relative;height:9007199254740991px;width:9007199254740991px;overflow:auto;-webkit-overflow-scrolling:touch;will-change:transform;direction:ltr" >
{ switchColumnPos: jest.fn(), schemaDetectors, rowHeightUtils: mockRowHeightUtils, + minRowHeight: 36, }; beforeAll(() => { Object.defineProperty(HTMLElement.prototype, 'offsetHeight', { configurable: true, - value: 34, + value: 36, }); }); diff --git a/src/components/datagrid/body/data_grid_body.tsx b/src/components/datagrid/body/data_grid_body.tsx index a8b3fd1e4f43..a99074edd158 100644 --- a/src/components/datagrid/body/data_grid_body.tsx +++ b/src/components/datagrid/body/data_grid_body.tsx @@ -28,7 +28,6 @@ import { useMutationObserver, } from '../../observer/mutation_observer'; import { useResizeObserver } from '../../observer/resize_observer'; -import { DEFAULT_ROW_HEIGHT } from '../row_height_utils'; import { EuiDataGridCell } from './data_grid_cell'; import { DataGridSortingContext, @@ -508,14 +507,15 @@ export const EuiDataGridBody: FunctionComponent = ( [rowHeightUtils] ); - const [minRowHeight, setRowHeight] = useState(DEFAULT_ROW_HEIGHT); + const [minRowHeight, setRowHeight] = useState(props.minRowHeight); useEffect(() => { // Reset the minimum row height when switching back to `undefined` rowHeightsOptions (e.g. via the display selector) + // This should also adjust the min height dynamically whenever density changes if (!rowHeightsOptions) { - setRowHeight(DEFAULT_ROW_HEIGHT); + setRowHeight(props.minRowHeight); } - }, [rowHeightsOptions]); + }, [rowHeightsOptions, props.minRowHeight]); const defaultHeight = useMemo(() => { return rowHeightsOptions?.defaultHeight diff --git a/src/components/datagrid/controls/display_selector.test.tsx b/src/components/datagrid/controls/display_selector.test.tsx index ed0fccb13373..107204ae4c37 100644 --- a/src/components/datagrid/controls/display_selector.test.tsx +++ b/src/components/datagrid/controls/display_selector.test.tsx @@ -15,7 +15,11 @@ import { EuiDataGridRowHeightsOptions, } from '../data_grid_types'; -import { useDataGridDisplaySelector, startingStyles } from './display_selector'; +import { + useDataGridDisplaySelector, + startingStyles, + densityRowHeightMap, +} from './display_selector'; describe('useDataGridDisplaySelector', () => { describe('displaySelector', () => { @@ -328,4 +332,42 @@ describe('useDataGridDisplaySelector', () => { expect(getOutput(component)).toEqual(''); }); }); + + describe('minRowHeight', () => { + // Test helpers + const MockComponent = () => { + const [displaySelector, , , minRowHeight] = useDataGridDisplaySelector( + true, + {}, + {} + ); + return ( + <> + {displaySelector} +
{JSON.stringify(minRowHeight)}
+ + ); + }; + const setDensity = (component: ShallowWrapper, selection = '') => { + const densityButtonGroup = (component + .find('EuiI18n') + .first() + .renderProp('children') as Function)(['', '', '', '']).find( + '[data-test-subj="densityButtonGroup"]' + ); + densityButtonGroup.simulate('change', selection); + }; + const getOutput = (component: ShallowWrapper) => { + return component.find('[data-test-subj="output"]').text(); + }; + + it('returns a minRowHeight value based on the current grid density', () => { + const component = shallow(); + + Object.entries(densityRowHeightMap).forEach(([density, minRowHeight]) => { + setDensity(component, density); + expect(Number(getOutput(component))).toEqual(minRowHeight); + }); + }); + }); }); diff --git a/src/components/datagrid/controls/display_selector.tsx b/src/components/datagrid/controls/display_selector.tsx index 04f0168225b5..80f700ab9291 100644 --- a/src/components/datagrid/controls/display_selector.tsx +++ b/src/components/datagrid/controls/display_selector.tsx @@ -32,6 +32,12 @@ export const startingStyles: EuiDataGridStyle = { stickyFooter: true, }; +export const densityRowHeightMap: { [key: string]: number } = { + compact: 24, + normal: 36, + expanded: 40, +}; + // These are the available options. They power the gridDensity hook and also the options in the render const densityOptions: string[] = ['compact', 'normal', 'expanded']; const densityStyles: { [key: string]: Partial } = { @@ -77,7 +83,8 @@ export const useDataGridDisplaySelector = ( ): [ ReactElement, EuiDataGridStyle, - EuiDataGridRowHeightsOptions | undefined + EuiDataGridRowHeightsOptions | undefined, + number ] => { const [isOpen, setIsOpen] = useState(false); @@ -150,6 +157,8 @@ export const useDataGridDisplaySelector = ( }; }, [initialRowHeightsOptions, userRowHeightsOptions]); + const minRowHeight = densityRowHeightMap[gridDensity]; + const buttonLabel = useEuiI18n( 'euiDisplaySelector.buttonText', 'Display options' @@ -288,5 +297,5 @@ export const useDataGridDisplaySelector = ( ); - return [displaySelector, gridStyles, rowHeightsOptions]; + return [displaySelector, gridStyles, rowHeightsOptions, minRowHeight]; }; diff --git a/src/components/datagrid/data_grid.test.tsx b/src/components/datagrid/data_grid.test.tsx index bd58ee30e25b..73ea96177728 100644 --- a/src/components/datagrid/data_grid.test.tsx +++ b/src/components/datagrid/data_grid.test.tsx @@ -539,7 +539,7 @@ describe('EuiDataGrid', () => { "role": "gridcell", "style": Object { "color": "red", - "height": 34, + "height": 36, "left": 0, "lineHeight": undefined, "position": "absolute", @@ -560,7 +560,7 @@ describe('EuiDataGrid', () => { "role": "gridcell", "style": Object { "color": "blue", - "height": 34, + "height": 36, "left": 100, "lineHeight": undefined, "position": "absolute", @@ -581,11 +581,11 @@ describe('EuiDataGrid', () => { "role": "gridcell", "style": Object { "color": "red", - "height": 34, + "height": 36, "left": 0, "lineHeight": undefined, "position": "absolute", - "top": "134px", + "top": "136px", "width": 100, }, "tabIndex": -1, @@ -602,11 +602,11 @@ describe('EuiDataGrid', () => { "role": "gridcell", "style": Object { "color": "blue", - "height": 34, + "height": 36, "left": 100, "lineHeight": undefined, "position": "absolute", - "top": "134px", + "top": "136px", "width": 100, }, "tabIndex": -1, @@ -894,8 +894,8 @@ describe('EuiDataGrid', () => { C: '$-5.80', E: '2019-09-18T12:31:28', F: '2019-09-18T12:31:28Z', - G: '2019-09-18T12:31:28.234', - H: '2019-09-18T12:31:28.234+0300', + G: '2019-09-18T12:31:28.236', + H: '2019-09-18T12:31:28.236+0300', }; const component = mount( { const cellHeights = extractRowHeights(component); expect(cellHeights).toEqual({ 0: 70, - 1: 34, + 1: 36, 2: 50, }); }); @@ -2262,7 +2262,7 @@ describe('EuiDataGrid', () => { expect(extractRowHeights(component)).toEqual({ 0: 70, - 1: 34, + 1: 36, 2: 50, }); @@ -2280,7 +2280,7 @@ describe('EuiDataGrid', () => { expect(extractRowHeights(component)).toEqual({ 0: 70, - 1: 34, + 1: 36, 2: 50, }); }); diff --git a/src/components/datagrid/data_grid.tsx b/src/components/datagrid/data_grid.tsx index 7d6c3bc1fce8..d3b76795eada 100644 --- a/src/components/datagrid/data_grid.tsx +++ b/src/components/datagrid/data_grid.tsx @@ -652,6 +652,7 @@ export const EuiDataGrid: FunctionComponent = (props) => { displaySelector, gridStyles, rowHeightsOptions, + minRowHeight, ] = useDataGridDisplaySelector( checkOrDefaultToolBarDisplayOptions( toolbarVisibility, @@ -892,6 +893,7 @@ export const EuiDataGrid: FunctionComponent = (props) => { rowHeightUtils={rowHeightUtils} virtualizationOptions={virtualizationOptions || {}} gridStyles={gridStyles} + minRowHeight={minRowHeight} />
diff --git a/src/components/datagrid/data_grid_types.ts b/src/components/datagrid/data_grid_types.ts index e81de27aab4d..25759ca2e9f7 100644 --- a/src/components/datagrid/data_grid_types.ts +++ b/src/components/datagrid/data_grid_types.ts @@ -332,6 +332,7 @@ export interface EuiDataGridBodyProps { rowHeightsOptions?: EuiDataGridRowHeightsOptions; rowHeightUtils: RowHeightUtils; gridStyles?: EuiDataGridStyle; + minRowHeight: number; } export interface EuiDataGridCellValueElementProps { /** diff --git a/src/components/datagrid/row_height_utils.test.ts b/src/components/datagrid/row_height_utils.test.ts index 66a22481d026..03f85bdaf84b 100644 --- a/src/components/datagrid/row_height_utils.test.ts +++ b/src/components/datagrid/row_height_utils.test.ts @@ -49,19 +49,19 @@ describe('RowHeightUtils', () => { describe('static height numbers', () => { it('returns the height number', () => { - expect(rowHeightUtils.getCalculatedHeight({ height: 100 }, 34)).toEqual( + expect(rowHeightUtils.getCalculatedHeight({ height: 100 }, 36)).toEqual( 100 ); }); it('returns passed numbers', () => { - expect(rowHeightUtils.getCalculatedHeight(100, 34)).toEqual(100); + expect(rowHeightUtils.getCalculatedHeight(100, 36)).toEqual(100); }); it('does not allow passing in static row heights shorter than the default/min row height', () => { - expect(rowHeightUtils.getCalculatedHeight(10, 34)).toEqual(34); - expect(rowHeightUtils.getCalculatedHeight({ height: 10 }, 34)).toEqual( - 34 + expect(rowHeightUtils.getCalculatedHeight(10, 36)).toEqual(36); + expect(rowHeightUtils.getCalculatedHeight({ height: 10 }, 36)).toEqual( + 36 ); }); }); @@ -71,12 +71,12 @@ describe('RowHeightUtils', () => { beforeEach(() => getRowHeightSpy.mockClear()); it('gets the max height for the current row from the heights cache', () => { - expect(rowHeightUtils.getCalculatedHeight('auto', 34, 1)).toEqual(0); // 0 is expected since the cache is empty + expect(rowHeightUtils.getCalculatedHeight('auto', 36, 1)).toEqual(0); // 0 is expected since the cache is empty expect(getRowHeightSpy).toHaveBeenCalled(); }); it('returns the default height when no rowIndex is passed', () => { - expect(rowHeightUtils.getCalculatedHeight('auto', 34)).toEqual(34); // row index is not passed when obtaining the initial defaultHeight for the entire grid + expect(rowHeightUtils.getCalculatedHeight('auto', 36)).toEqual(36); // row index is not passed when obtaining the initial defaultHeight for the entire grid expect(getRowHeightSpy).not.toHaveBeenCalled(); }); }); @@ -85,9 +85,9 @@ describe('RowHeightUtils', () => { describe('invalid row height configs', () => { it('returns the default height', () => { // @ts-expect-error intentionally incorrect - expect(rowHeightUtils.getCalculatedHeight({}, 34)).toEqual(34); + expect(rowHeightUtils.getCalculatedHeight({}, 36)).toEqual(36); // @ts-expect-error intentionally incorrect - expect(rowHeightUtils.getCalculatedHeight('invalid', 34)).toEqual(34); + expect(rowHeightUtils.getCalculatedHeight('invalid', 36)).toEqual(36); }); }); }); @@ -130,7 +130,7 @@ describe('RowHeightUtils', () => { describe('numeric heights', () => { it('returns default CSS', () => { expect( - rowHeightUtils.getStylesForCell({ defaultHeight: 34 }, 0) + rowHeightUtils.getStylesForCell({ defaultHeight: 36 }, 0) ).toEqual({ height: '100%', overflow: 'hidden' }); }); }); @@ -146,8 +146,8 @@ describe('RowHeightUtils', () => { it('returns undefined for non-lineCount configs', () => { expect(rowHeightUtils.getLineCount(undefined)).toEqual(undefined); expect(rowHeightUtils.getLineCount('auto')).toEqual(undefined); - expect(rowHeightUtils.getLineCount(34)).toEqual(undefined); - expect(rowHeightUtils.getLineCount({ height: 34 })).toEqual(undefined); + expect(rowHeightUtils.getLineCount(36)).toEqual(undefined); + expect(rowHeightUtils.getLineCount({ height: 36 })).toEqual(undefined); }); }); @@ -188,13 +188,13 @@ describe('RowHeightUtils', () => { it('returns false otherwise', () => { expect( rowHeightUtils.isAutoHeight(1, { - rowHeights: { 1: 34 }, + rowHeights: { 1: 36 }, defaultHeight: 'auto', }) ).toEqual(false); expect( rowHeightUtils.isAutoHeight(1, { - defaultHeight: 34, + defaultHeight: 36, }) ).toEqual(false); }); @@ -207,15 +207,15 @@ describe('RowHeightUtils', () => { it('setRowHeight', () => { rowHeightUtils.setRowHeight(5, 'a', 50, 0); - rowHeightUtils.setRowHeight(5, 'b', 34, 0); + rowHeightUtils.setRowHeight(5, 'b', 36, 0); rowHeightUtils.setRowHeight(5, 'c', 100, 0); rowHeightUtils.setRowHeight(5, 'd', undefined, 0); // @ts-ignore this var is private, but we're inspecting it for the sake of the unit test expect(rowHeightUtils.heightsCache.get(5)?.get('a')).toEqual(62); // @ts-ignore-line - expect(rowHeightUtils.heightsCache.get(5)?.get('b')).toEqual(46); // @ts-ignore-line + expect(rowHeightUtils.heightsCache.get(5)?.get('b')).toEqual(48); // @ts-ignore-line expect(rowHeightUtils.heightsCache.get(5)?.get('c')).toEqual(112); // @ts-ignore-line - expect(rowHeightUtils.heightsCache.get(5)?.get('d')).toEqual(46); // Falls back default row height + expect(rowHeightUtils.heightsCache.get(5)?.get('d')).toEqual(12); // Falls back to 0 // NB: The cached heights have padding added to them expect(resetRowSpy).toHaveBeenCalledTimes(4); @@ -291,14 +291,14 @@ describe('RowHeightUtils', () => { describe('resetGrid', () => { it('invokes grid.resetAfterRowIndex with the last visible row', () => { - rowHeightUtils.setRowHeight(99, 'a', 34, 99); + rowHeightUtils.setRowHeight(99, 'a', 36, 99); rowHeightUtils.resetGrid(); expect(mockGrid.resetAfterRowIndex).toHaveBeenCalledWith(99); }); it('invokes resetAfterRowIndex only once with the smallest cached row index', () => { - rowHeightUtils.setRowHeight(97, 'a', 34, 97); - rowHeightUtils.setRowHeight(99, 'a', 34, 99); + rowHeightUtils.setRowHeight(97, 'a', 36, 97); + rowHeightUtils.setRowHeight(99, 'a', 36, 99); rowHeightUtils.resetGrid(); expect(mockGrid.resetAfterRowIndex).toHaveBeenCalledTimes(1); expect(mockGrid.resetAfterRowIndex).toHaveBeenCalledWith(97); diff --git a/src/components/datagrid/row_height_utils.ts b/src/components/datagrid/row_height_utils.ts index 383003efe23c..a99a3352807d 100644 --- a/src/components/datagrid/row_height_utils.ts +++ b/src/components/datagrid/row_height_utils.ts @@ -25,7 +25,6 @@ export const cellPaddingsMap: Record = { }; export const AUTO_HEIGHT = 'auto'; -export const DEFAULT_ROW_HEIGHT = 34; export class RowHeightUtils { getRowHeightOption( @@ -162,7 +161,7 @@ export class RowHeightUtils { setRowHeight( rowIndex: number, colId: string, - height: number = DEFAULT_ROW_HEIGHT, + height: number = 0, visibleRowIndex: number ) { const rowHeights =