From b0d45b2b98b4a1026f1ab344dffb4d9df53bb52a Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Thu, 28 Mar 2024 16:44:32 -0700 Subject: [PATCH 1/6] Fix strut issue in action cells - I apparently already fixed this at some point at the EuiBasicTable level - this fixes it for all table usages now + bonus - remove unnecessary function in `safariLoadingWorkaround` --- .../basic_table/__snapshots__/basic_table.test.tsx.snap | 6 +++--- src/components/basic_table/basic_table.styles.ts | 8 +------- src/components/basic_table/basic_table.tsx | 2 -- src/components/table/table_row_cell.styles.ts | 6 ++++++ src/components/table/table_row_cell.tsx | 1 + 5 files changed, 11 insertions(+), 12 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 42549e0c5a5..fc7f14fb9ad 100644 --- a/src/components/basic_table/__snapshots__/basic_table.test.tsx.snap +++ b/src/components/basic_table/__snapshots__/basic_table.test.tsx.snap @@ -327,7 +327,7 @@ exports[`EuiBasicTable renders (kitchen sink) with pagination, selection, sortin
css` // Fix to make the loading indicator position correctly in Safari // For whatever annoying reason, Safari doesn't respect `position: relative;` // on `tbody` without `position: relative` on the parent `table` -export const safariLoadingWorkaround = () => css` +export const safariLoadingWorkaround = css` position: relative; `; - -// Unsets the extra height caused by tooltip/popover wrappers around table action buttons -// Without this, the row height jumps whenever actions are disabled -export const euiBasicTableActionsWrapper = css` - line-height: 1; -`; diff --git a/src/components/basic_table/basic_table.tsx b/src/components/basic_table/basic_table.tsx index 75b9e6b542f..b0758c00037 100644 --- a/src/components/basic_table/basic_table.tsx +++ b/src/components/basic_table/basic_table.tsx @@ -78,7 +78,6 @@ import { EuiTableSortMobileProps } from '../table/mobile/table_sort_mobile'; import { euiBasicTableBodyLoading, safariLoadingWorkaround, - euiBasicTableActionsWrapper, } from './basic_table.styles'; type DataTypeProfiles = Record< @@ -1174,7 +1173,6 @@ export class EuiBasicTable extends Component< align="right" textOnly={false} hasActions={hasCustomActions ? 'custom' : true} - css={euiBasicTableActionsWrapper} > { color: ${euiTheme.colors.text}; `, + hasActions: css` + /* Unsets the extra strut caused by inline-block display of buttons/icons/tooltips. + Without this, the row height jumps whenever actions are disabled. */ + line-height: 1; + `, + // valign middle: css` vertical-align: middle; diff --git a/src/components/table/table_row_cell.tsx b/src/components/table/table_row_cell.tsx index 3586f6b9172..bad7603207a 100644 --- a/src/components/table/table_row_cell.tsx +++ b/src/components/table/table_row_cell.tsx @@ -137,6 +137,7 @@ export const EuiTableRowCell: FunctionComponent = ({ const styles = useEuiMemoizedStyles(euiTableRowCellStyles); const cssStyles = [ styles.euiTableRowCell, + hasActions && styles.hasActions, styles[valign], ...(isResponsive ? [ From 4df25f2f4dba97988192049fc9ace7cbfcb9b5e1 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Mon, 1 Apr 2024 11:31:10 -0700 Subject: [PATCH 2/6] Convert initial styles for desktop action cells - ignore `showOnHover` distinction for now - we should allow both default actions and custom actions to have hover CSS + add flex wrapping for responsive scenarios where the table hasn't yet collapsed to mobile (which means icons may be overflowing out of small cells) + clarify prop docs --- src/components/table/_table.scss | 6 ------ src/components/table/table_row.tsx | 4 ++-- src/components/table/table_row_cell.styles.ts | 21 ++++++++++++++++--- src/components/table/table_row_cell.tsx | 6 +++--- 4 files changed, 23 insertions(+), 14 deletions(-) diff --git a/src/components/table/_table.scss b/src/components/table/_table.scss index 6ef3c9a0c98..bb5373a0b6d 100644 --- a/src/components/table/_table.scss +++ b/src/components/table/_table.scss @@ -43,12 +43,6 @@ word-break: break-word; } -.euiTableCellContent--showOnHover { - > *:not(:first-child) { - margin-left: $euiSizeS; - } -} - .euiTableRow-hasActions { .euiTableCellContent--showOnHover { .euiTableCellContent__hoverItem { diff --git a/src/components/table/table_row.tsx b/src/components/table/table_row.tsx index 95840f0384f..05830f37924 100644 --- a/src/components/table/table_row.tsx +++ b/src/components/table/table_row.tsx @@ -35,8 +35,8 @@ export interface EuiTableRowProps { */ isSelected?: boolean; /** - * Indicates if the table has a dedicated column for icon-only actions - * (used for mobile styling) + * Indicates if the table has a dedicated column for actions + * (used for mobile styling and desktop action hover behavior) */ hasActions?: boolean; /** diff --git a/src/components/table/table_row_cell.styles.ts b/src/components/table/table_row_cell.styles.ts index 5b6dcc99be7..cb85407f270 100644 --- a/src/components/table/table_row_cell.styles.ts +++ b/src/components/table/table_row_cell.styles.ts @@ -27,6 +27,13 @@ export const euiTableRowCellStyles = (euiThemeContext: UseEuiTheme) => { /* Unsets the extra strut caused by inline-block display of buttons/icons/tooltips. Without this, the row height jumps whenever actions are disabled. */ line-height: 1; + + /* TODO: Move this to EuiTableCellContent, once we're further along in the Emotion conversion */ + .euiTableCellContent { + display: flex; + align-items: center; + gap: ${euiTheme.size.s}; + } `, // valign @@ -43,9 +50,17 @@ export const euiTableRowCellStyles = (euiThemeContext: UseEuiTheme) => { vertical-align: bottom; `, - desktop: css` - ${logicalCSS('border-vertical', euiTheme.border.thin)} - `, + desktop: { + desktop: css` + ${logicalCSS('border-vertical', euiTheme.border.thin)} + `, + actions: css` + /* TODO: Move this to EuiTableCellContent, once we're further along in the Emotion conversion */ + .euiTableCellContent { + flex-wrap: wrap; + } + `, + }, mobile: { mobile: css` diff --git a/src/components/table/table_row_cell.tsx b/src/components/table/table_row_cell.tsx index bad7603207a..e1224fd94de 100644 --- a/src/components/table/table_row_cell.tsx +++ b/src/components/table/table_row_cell.tsx @@ -95,8 +95,8 @@ export interface EuiTableRowCellProps extends EuiTableRowCellSharedPropsShape { */ setScopeRow?: boolean; /** - * Indicates if the column is dedicated to icon-only actions (currently - * affects mobile only) + * Indicates if the cell is dedicated to row actions + * (used for mobile styling and desktop action hover behavior) */ hasActions?: boolean | 'custom'; /** @@ -147,7 +147,7 @@ export const EuiTableRowCell: FunctionComponent = ({ hasActions === true && styles.mobile.actions, isExpander && styles.mobile.expander, ] - : [styles.desktop]), + : [styles.desktop.desktop, hasActions && styles.desktop.actions]), ]; const cellClasses = classNames('euiTableRowCell', className, { From 2c6f412a78dadf54bbeccba301458f3783d5743b Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Mon, 1 Apr 2024 11:51:09 -0700 Subject: [PATCH 3/6] Clean up/convert responsive actions hover CSS to Emotion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Ignore `showOnHover` prop/CSS completely and have the expected hover className be set by `EuiBasicTable` rather than by table cell(s) themselves - Fix primary actions being hidden on mobile even though the comment indicates they should be visible - Remove entirely unnecessary grayscale filter - not used by the current theme or visible to the naked eye as far as I can tell ⚠️ Removes the following classNames: - `.expandedItemActions__completelyHide` (not correct and no longer necessary as far as I can tell, since we splice the actions array pre-emptively via JSX in basic tables) --- changelogs/upcoming/7640.md | 3 +++ .../basic_table/expanded_item_actions.tsx | 2 +- src/components/table/_responsive.scss | 18 -------------- src/components/table/_table.scss | 24 ------------------- src/components/table/table_row_cell.styles.ts | 14 +++++++++++ 5 files changed, 18 insertions(+), 43 deletions(-) create mode 100644 changelogs/upcoming/7640.md diff --git a/changelogs/upcoming/7640.md b/changelogs/upcoming/7640.md new file mode 100644 index 00000000000..3f0ca262a20 --- /dev/null +++ b/changelogs/upcoming/7640.md @@ -0,0 +1,3 @@ +**Bug fixes** + +- `EuiBasicTable` & `EuiInMemoryTable` `isPrimary` actions are now correctly shown on mobile views diff --git a/src/components/basic_table/expanded_item_actions.tsx b/src/components/basic_table/expanded_item_actions.tsx index aa07ebece8c..b645cff1482 100644 --- a/src/components/basic_table/expanded_item_actions.tsx +++ b/src/components/basic_table/expanded_item_actions.tsx @@ -48,7 +48,7 @@ export const ExpandedItemActions = ({ const key = `item_action_${itemId}_${index}`; const classes = classNames(className, { - expandedItemActions__completelyHide: moreThanThree && index < 2, + 'euiBasicTableAction-showOnHover': moreThanThree && index < 2, }); if (isCustomItemAction(action)) { diff --git a/src/components/table/_responsive.scss b/src/components/table/_responsive.scss index 2ebcdd8034f..0c91e7acc56 100644 --- a/src/components/table/_responsive.scss +++ b/src/components/table/_responsive.scss @@ -3,24 +3,6 @@ @include euiBreakpoint('xs', 's') { .euiTable.euiTable--responsive { - // never show hidden items and always show hover items on mobile, - .euiTableRow-hasActions .euiTableCellContent--showOnHover { - > * { - margin-left: 0; - } - - .expandedItemActions__completelyHide { - display: none; - } - - .euiTableCellContent__hoverItem { - opacity: 1; - filter: none; - margin-left: 0; - margin-bottom: $euiSizeS; - } - } - // force all content back to left side .euiTableCellContent--alignRight { justify-content: flex-start; diff --git a/src/components/table/_table.scss b/src/components/table/_table.scss index bb5373a0b6d..c8fd7333225 100644 --- a/src/components/table/_table.scss +++ b/src/components/table/_table.scss @@ -42,27 +42,3 @@ // /* 4 */ overflow-wrap is not supported on flex parents word-break: break-word; } - -.euiTableRow-hasActions { - .euiTableCellContent--showOnHover { - .euiTableCellContent__hoverItem { - flex-shrink: 0; - opacity: .7; - filter: grayscale(100%); - transition: opacity $euiAnimSpeedNormal $euiAnimSlightResistance, filter $euiAnimSpeedNormal $euiAnimSlightResistance; - } - - .expandedItemActions__completelyHide { - filter: grayscale(0%); - opacity: 0; - } - } - - &:hover .euiTableCellContent--showOnHover, - .euiTableCellContent--showOnHover:focus-within { - .euiTableCellContent__hoverItem { - opacity: 1; - filter: grayscale(0%); - } - } -} diff --git a/src/components/table/table_row_cell.styles.ts b/src/components/table/table_row_cell.styles.ts index cb85407f270..9ef85a0f70c 100644 --- a/src/components/table/table_row_cell.styles.ts +++ b/src/components/table/table_row_cell.styles.ts @@ -59,6 +59,19 @@ export const euiTableRowCellStyles = (euiThemeContext: UseEuiTheme) => { .euiTableCellContent { flex-wrap: wrap; } + + .euiBasicTableAction-showOnHover { + opacity: 0; + transition: opacity ${euiTheme.animation.normal} + ${euiTheme.animation.resistance}; + } + + &:focus-within, + .euiTableRow-hasActions:hover & { + .euiBasicTableAction-showOnHover { + opacity: 1; + } + } `, }, @@ -85,6 +98,7 @@ export const euiTableRowCellStyles = (euiThemeContext: UseEuiTheme) => { } `, get actions() { + // Note: Visible-on-hover actions on desktop always show on mobile return css` ${this.rightColumnContent} ${logicalCSS('top', mobileSizes.actions.offset)} From 14b17f790dfa3b5a4493f12d651435f2f69e4d07 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Mon, 1 Apr 2024 11:56:57 -0700 Subject: [PATCH 4/6] =?UTF-8?q?=F0=9F=94=A5=20Remove=20the=20`showOnHover`?= =?UTF-8?q?=20prop=20from=20EuiTableRowCell?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Per prior commit, this should be handled for consumers via EuiBasicTable API rather than by the cell component API ⚠️ Removes the following classNames: - `.euiTableCellContent__hoverItem` - `.euiTableCellContent--showOnHover` --- .../__snapshots__/basic_table.test.tsx.snap | 18 +++++++++--------- src/components/basic_table/basic_table.tsx | 1 - .../__snapshots__/table_row_cell.test.tsx.snap | 4 ++-- src/components/table/table_row_cell.test.tsx | 2 +- src/components/table/table_row_cell.tsx | 9 --------- 5 files changed, 12 insertions(+), 22 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 fc7f14fb9ad..177c5f5a488 100644 --- a/src/components/basic_table/__snapshots__/basic_table.test.tsx.snap +++ b/src/components/basic_table/__snapshots__/basic_table.test.tsx.snap @@ -330,13 +330,13 @@ exports[`EuiBasicTable renders (kitchen sink) with pagination, selection, sortin class="euiTableRowCell euiTableRowCell--hasActions emotion-euiTableRowCell-hasActions-middle-desktop-actions" >
+ + `; diff --git a/src/components/basic_table/action_types.ts b/src/components/basic_table/action_types.ts index cb8d6981cbf..1ac557ee5dd 100644 --- a/src/components/basic_table/action_types.ts +++ b/src/components/basic_table/action_types.ts @@ -39,8 +39,21 @@ export interface DefaultItemActionBase { * A callback function that determines whether the action is enabled */ enabled?: (item: T) => boolean; - isPrimary?: boolean; 'data-test-subj'?: string | ((item: T) => string); + /** + * If more than 3 actions are passed, 2 primary actions will show (on hover) + * next to an expansion menu of all actions. + * + * On mobile, primary actions will be tucked away in the expansion menu for space. + */ + isPrimary?: boolean; + /** + * Allows only showing the action on mouse hover or keyboard focus. + * If more than 3 actions are passed, this will always be true for `isPrimary` actions. + * + * Has no effect on mobile, or if `hasActions` is not set. + */ + showOnHover?: boolean; } export interface DefaultItemEmptyButtonAction @@ -70,7 +83,7 @@ export type DefaultItemAction = ExclusiveUnion< DefaultItemIconButtonAction >; -export interface CustomItemAction { +export type CustomItemAction = { /** * Allows rendering a totally custom action */ @@ -83,8 +96,7 @@ export interface CustomItemAction { * A callback that defines whether the action is enabled */ enabled?: (item: T) => boolean; - isPrimary?: boolean; -} +} & Pick, 'isPrimary' | 'showOnHover'>; export type Action = | DefaultItemAction diff --git a/src/components/basic_table/basic_table.test.tsx b/src/components/basic_table/basic_table.test.tsx index 8db29197dac..49d9eae7ac3 100644 --- a/src/components/basic_table/basic_table.test.tsx +++ b/src/components/basic_table/basic_table.test.tsx @@ -664,9 +664,12 @@ describe('EuiBasicTable', () => { }, ], }; - const { getAllByText } = render(); + const { getAllByText, container } = render(); expect(getAllByText('Delete')).toHaveLength(basicItems.length); + expect( + container.querySelector('.euiBasicTableAction-showOnHover') + ).not.toBeInTheDocument(); }); test('multiple actions with custom availability', () => { @@ -680,7 +683,7 @@ describe('EuiBasicTable', () => { }, ], }; - const { getAllByText, getAllByTestSubject } = render( + const { getAllByText, getAllByTestSubject, container } = render( ); @@ -689,6 +692,12 @@ describe('EuiBasicTable', () => { expect(getAllByTestSubject('euiCollapsedItemActionsButton')).toHaveLength( 4 ); + expect( + container.querySelector('.euiBasicTable__collapsedActions') + ).toBeInTheDocument(); + expect( + container.querySelector('.euiBasicTableAction-showOnHover') + ).toBeInTheDocument(); }); test('custom item actions', () => { diff --git a/src/components/basic_table/basic_table.tsx b/src/components/basic_table/basic_table.tsx index 9921e11b4f1..2b060f15831 100644 --- a/src/components/basic_table/basic_table.tsx +++ b/src/components/basic_table/basic_table.tsx @@ -1139,9 +1139,15 @@ export class EuiBasicTable extends Component< // If all actions are disabled, do not show any actions but the popover toggle actualActions = []; } else { - // if any of the actions `isPrimary`, add them inline as well, but only the first 2 - const primaryActions = actualActions.filter((o) => o.isPrimary); - actualActions = primaryActions.slice(0, 2); + // if any of the actions `isPrimary`, add them inline as well, but only the first 2, + // which we'll force to only show on hover for desktop views + const primaryActions = actualActions.filter( + (action) => action.isPrimary + ); + actualActions = primaryActions.slice(0, 2).map((action) => ({ + ...action, + showOnHover: true, + })); } // if we have more than 1 action, we don't show them all in the cell, instead we @@ -1152,16 +1158,15 @@ export class EuiBasicTable extends Component< actualActions.push({ name: 'All actions', - render: (item: T) => { - return ( - - ); - }, + render: (item: T) => ( + + ), }); } diff --git a/src/components/basic_table/expanded_item_actions.test.tsx b/src/components/basic_table/expanded_item_actions.test.tsx index 99118a9de92..6a23ee0888f 100644 --- a/src/components/basic_table/expanded_item_actions.test.tsx +++ b/src/components/basic_table/expanded_item_actions.test.tsx @@ -7,14 +7,15 @@ */ import React from 'react'; -import { shallow } from 'enzyme'; +import { render } from '../../test/rtl'; + import { ExpandedItemActions, ExpandedItemActionsProps, } from './expanded_item_actions'; describe('ExpandedItemActions', () => { - test('render', () => { + it('renders', () => { const props: ExpandedItemActionsProps<{ id: string }> = { actions: [ { @@ -27,14 +28,19 @@ describe('ExpandedItemActions', () => { description: 'custom 1', render: (_item) => <>, }, + { + name: 'showOnHover', + description: 'show on hover', + href: '#', + showOnHover: true, + }, ], itemId: 'xyz', item: { id: 'xyz' }, actionsDisabled: false, }; + const { container } = render(); - const component = shallow(); - - expect(component).toMatchSnapshot(); + expect(container).toMatchSnapshot(); }); }); diff --git a/src/components/basic_table/expanded_item_actions.tsx b/src/components/basic_table/expanded_item_actions.tsx index b645cff1482..3fd319cb0be 100644 --- a/src/components/basic_table/expanded_item_actions.tsx +++ b/src/components/basic_table/expanded_item_actions.tsx @@ -33,8 +33,6 @@ export const ExpandedItemActions = ({ actionsDisabled, className, }: ExpandedItemActionsProps): ReactElement => { - const moreThanThree = actions.length > 2; - return ( <> {actions.reduce((tools, action, index) => { @@ -48,7 +46,7 @@ export const ExpandedItemActions = ({ const key = `item_action_${itemId}_${index}`; const classes = classNames(className, { - 'euiBasicTableAction-showOnHover': moreThanThree && index < 2, + 'euiBasicTableAction-showOnHover': action.showOnHover, }); if (isCustomItemAction(action)) { From 22b40da01192dbee9acbccbcd432d648dda0fa13 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Tue, 2 Apr 2024 09:34:52 -0700 Subject: [PATCH 6/6] Fix strut CSS for expander cells as well --- src/components/table/table_row_cell.styles.ts | 12 ++++++++---- src/components/table/table_row_cell.tsx | 1 + 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/components/table/table_row_cell.styles.ts b/src/components/table/table_row_cell.styles.ts index 9ef85a0f70c..d68b90414ac 100644 --- a/src/components/table/table_row_cell.styles.ts +++ b/src/components/table/table_row_cell.styles.ts @@ -18,15 +18,19 @@ export const euiTableRowCellStyles = (euiThemeContext: UseEuiTheme) => { const { mobileSizes } = euiTableVariables(euiThemeContext); + // Unsets the extra strut caused by inline-block display of buttons/icons/tooltips. + // Without this, the row height jumps whenever actions are disabled. + const hasIcons = `line-height: 1;`; + return { euiTableRowCell: css` color: ${euiTheme.colors.text}; `, - + isExpander: css` + ${hasIcons} + `, hasActions: css` - /* Unsets the extra strut caused by inline-block display of buttons/icons/tooltips. - Without this, the row height jumps whenever actions are disabled. */ - line-height: 1; + ${hasIcons} /* TODO: Move this to EuiTableCellContent, once we're further along in the Emotion conversion */ .euiTableCellContent { diff --git a/src/components/table/table_row_cell.tsx b/src/components/table/table_row_cell.tsx index ba9685550d2..a607e6c92cd 100644 --- a/src/components/table/table_row_cell.tsx +++ b/src/components/table/table_row_cell.tsx @@ -132,6 +132,7 @@ export const EuiTableRowCell: FunctionComponent = ({ const styles = useEuiMemoizedStyles(euiTableRowCellStyles); const cssStyles = [ styles.euiTableRowCell, + isExpander && styles.isExpander, hasActions && styles.hasActions, styles[valign], ...(isResponsive