From 4b89eaf3473fd616f846f6fc383c6792ae200b9d Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Mon, 1 Apr 2024 22:06:05 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fix=20custom=20action=20mobile?= =?UTF-8?q?=20styling?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix 1: `flex-direction: column` and `padding: 0` was being applied to custom action cells when they should not have been - Fix 2: table rows with only custom actions were still showing an empty right column border/padding on mobile. We needed the `actions="custom"` logic on table rows as well as row cells + Add a few test assertions to try and capture regressions in the future --- .../__snapshots__/basic_table.test.tsx.snap | 24 +++++++++++++++++++ .../basic_table/basic_table.test.tsx | 13 +++++++++- src/components/basic_table/basic_table.tsx | 23 +++++++++++------- src/components/table/_table_cell_content.tsx | 10 +++++--- src/components/table/table_row.tsx | 4 ++-- 5 files changed, 59 insertions(+), 15 deletions(-) diff --git a/src/components/basic_table/__snapshots__/basic_table.test.tsx.snap b/src/components/basic_table/__snapshots__/basic_table.test.tsx.snap index 9a6fdad1183e..32cae7404862 100644 --- a/src/components/basic_table/__snapshots__/basic_table.test.tsx.snap +++ b/src/components/basic_table/__snapshots__/basic_table.test.tsx.snap @@ -1,5 +1,29 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`EuiBasicTable actions custom item actions 1`] = ` + + +
+
+ +
+
+ + +`; + exports[`EuiBasicTable renders (bare-bones) 1`] = `
{ const props: EuiBasicTableProps = { items: basicItems, columns: [ - ...basicColumns, { name: 'Actions', actions: [ @@ -729,9 +728,21 @@ describe('EuiBasicTable', () => { expect(queryByTestSubject('customAction-2')).toBeInTheDocument(); expect(queryByTestSubject('customAction-3')).not.toBeInTheDocument(); + // TODO: These assertions should ideally be visual regression snapshots instead expect( container.querySelector('.euiTableRowCell--hasActions')!.className ).toContain('-customActions'); + expect( + container.querySelector( + '.euiTableRowCell--hasActions .euiTableCellContent' + )!.className + ).not.toContain('-actions-mobile'); + expect( + container.querySelector('.euiTableRow-hasActions')!.className + ).not.toContain('-hasRightColumn'); + expect( + container.querySelector('.euiTableRow-hasActions') + ).toMatchSnapshot(); }); describe('are disabled on selection', () => { diff --git a/src/components/basic_table/basic_table.tsx b/src/components/basic_table/basic_table.tsx index 2b060f158318..b486d9409db2 100644 --- a/src/components/basic_table/basic_table.tsx +++ b/src/components/basic_table/basic_table.tsx @@ -987,18 +987,26 @@ export class EuiBasicTable extends Component< rowSelectionDisabled = !!isDisabled; } - let hasActions; + let hasActions: 'custom' | boolean = false; columns.forEach((column: EuiBasicTableColumn, columnIndex: number) => { - if ((column as EuiTableActionsColumnType).actions) { + const columnActions = (column as EuiTableActionsColumnType).actions; + + if (columnActions) { + const hasCustomActions = columnActions.some( + (action) => !!(action as CustomItemAction).render + ); cells.push( this.renderItemActionsCell( itemId, item, column as EuiTableActionsColumnType, - columnIndex + columnIndex, + hasCustomActions ) ); - hasActions = true; + // A table theoretically could have both custom and default action items + // If it has both, default action mobile row styles take precedence over custom + hasActions = !hasActions && hasCustomActions ? 'custom' : true; } else if ((column as EuiTableFieldDataColumnType).field) { const fieldDataColumn = column as EuiTableFieldDataColumnType; cells.push( @@ -1122,15 +1130,12 @@ export class EuiBasicTable extends Component< itemId: ItemIdResolved, item: T, column: EuiTableActionsColumnType, - columnIndex: number + columnIndex: number, + hasCustomActions: boolean ) { // Disable all actions if any row(s) are selected const allDisabled = this.state.selection.length > 0; - const hasCustomActions = column.actions.some( - (action: Action) => !!(action as CustomItemAction).render - ); - let actualActions = column.actions.filter( (action: Action) => !action.available || action.available(item) ); diff --git a/src/components/table/_table_cell_content.tsx b/src/components/table/_table_cell_content.tsx index 887f2414b39d..2abc04b5f48b 100644 --- a/src/components/table/_table_cell_content.tsx +++ b/src/components/table/_table_cell_content.tsx @@ -43,9 +43,13 @@ export const EuiTableCellContent: FunctionComponent< !isResponsive && styles[align], // On mobile, always align cells to the left truncateText === true && styles.truncateText, truncateText === false && styles.wrapText, - hasActions && styles.hasActions.actions, - hasActions && - (isResponsive ? styles.hasActions.mobile : styles.hasActions.desktop), + ...(hasActions + ? [ + styles.hasActions.actions, + !isResponsive && styles.hasActions.desktop, + isResponsive && hasActions !== 'custom' && styles.hasActions.mobile, + ] + : []), ]; const classes = classNames('euiTableCellContent', className); diff --git a/src/components/table/table_row.tsx b/src/components/table/table_row.tsx index 05830f379243..73d48abf76ab 100644 --- a/src/components/table/table_row.tsx +++ b/src/components/table/table_row.tsx @@ -38,7 +38,7 @@ export interface EuiTableRowProps { * Indicates if the table has a dedicated column for actions * (used for mobile styling and desktop action hover behavior) */ - hasActions?: boolean; + hasActions?: boolean | 'custom'; /** * Indicates if the row will have an expanded row */ @@ -75,7 +75,7 @@ export const EuiTableRow: FunctionComponent = ({ styles.mobile.mobile, isSelected && styles.mobile.selected, isExpandedRow && styles.mobile.expanded, - (hasActions || isExpandable || isExpandedRow) && + (hasActions === true || isExpandable || isExpandedRow) && styles.mobile.hasRightColumn, hasSelection && styles.mobile.hasLeftColumn, ]