From c5858b115fe23ee55a81081e7a7b86983070b3ef Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Tue, 14 Nov 2023 00:51:20 -0800 Subject: [PATCH 1/8] [docs] Fix table actions example not rendering the collapsed toggle for custom actions --- src-docs/src/views/tables/actions/actions.tsx | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src-docs/src/views/tables/actions/actions.tsx b/src-docs/src/views/tables/actions/actions.tsx index 86ac089a0af..a233c872ac1 100644 --- a/src-docs/src/views/tables/actions/actions.tsx +++ b/src-docs/src/views/tables/actions/actions.tsx @@ -171,6 +171,11 @@ export default () => { ); }, }, + { + render: () => { + return {}}>Edit; + }, + }, ...actions, ]; } From cd5f6ac99aebfafcd7bfc21f5a8728a4aabf3322 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Tue, 14 Nov 2023 00:52:13 -0800 Subject: [PATCH 2/8] Fix custom actions to not render a ` + + + +
+
+
+ +
+
+ +`; + +exports[`CollapsedItemActions default actions 1`] = ` + +
+
+ + + +
+
+
+
+
+ +
+
+ +`; + +exports[`CollapsedItemActions renders 1`] = `
`; - -exports[`CollapsedItemActions render with href and _target provided 1`] = ` - - [Function] - - } - closePopover={[Function]} - display="inline-block" - hasArrow={true} - id="id-actions" - isOpen={true} - ownFocus={true} - panelPaddingSize="none" - popoverRef={[Function]} - repositionToCrossAxis={true} -> - - default1 - , - - -
- - , - - default2 - , - ] - } - /> - -`; diff --git a/src/components/basic_table/collapsed_item_actions.test.tsx b/src/components/basic_table/collapsed_item_actions.test.tsx index 591a03d5d3e..f369e459f9f 100644 --- a/src/components/basic_table/collapsed_item_actions.test.tsx +++ b/src/components/basic_table/collapsed_item_actions.test.tsx @@ -6,14 +6,18 @@ * Side Public License, v 1. */ -import React, { FocusEvent } from 'react'; -import { shallow } from 'enzyme'; -import { render } from '../../test/rtl'; +import React from 'react'; +import { fireEvent } from '@testing-library/react'; +import { + render, + waitForEuiPopoverOpen, + waitForEuiPopoverClose, +} from '../../test/rtl'; + import { CollapsedItemActions } from './collapsed_item_actions'; -import { Action } from './action_types'; describe('CollapsedItemActions', () => { - test('render', () => { + it('renders', () => { const props = { actions: [ { @@ -29,9 +33,7 @@ describe('CollapsedItemActions', () => { ], itemId: 'id', item: { id: '1' }, - actionEnabled: (_: Action<{ id: string }>) => true, - onFocus: (_: FocusEvent) => {}, - onBlur: () => {}, + actionEnabled: () => true, }; const { container } = render(); @@ -39,18 +41,14 @@ describe('CollapsedItemActions', () => { expect(container.firstChild).toMatchSnapshot(); }); - test('render with href and _target provided', () => { + test('default actions', async () => { const props = { actions: [ { name: 'default1', description: 'default 1', onClick: () => {}, - }, - { - name: 'custom1', - description: 'custom 1', - render: () =>
, + 'data-test-subj': 'defaultAction', }, { name: 'default2', @@ -61,14 +59,45 @@ describe('CollapsedItemActions', () => { ], itemId: 'id', item: { id: 'xyz' }, - actionEnabled: (_: Action<{ id: string }>) => true, - onFocus: (_: FocusEvent) => {}, - onBlur: () => {}, + actionEnabled: () => true, + }; + + const { getByTestSubject, baseElement } = render( + + ); + fireEvent.click(getByTestSubject('euiCollapsedItemActionsButton')); + await waitForEuiPopoverOpen(); + + expect(baseElement).toMatchSnapshot(); + + fireEvent.click(getByTestSubject('defaultAction')); + await waitForEuiPopoverClose(); + }); + + test('custom actions', async () => { + const props = { + actions: [ + { render: () => }, + { render: () => world }, + ], + itemId: 'id', + item: { id: 'xyz' }, + actionEnabled: () => true, }; - const component = shallow(); - component.setState({ popoverOpen: true }); + const { getByTestSubject, baseElement } = render( + + ); + fireEvent.click(getByTestSubject('euiCollapsedItemActionsButton')); + await waitForEuiPopoverOpen(); + + expect( + baseElement.querySelector('.euiBasicTable__collapsedCustomAction') + ?.nodeName + ).toEqual('DIV'); + expect(baseElement).toMatchSnapshot(); - expect(component).toMatchSnapshot(); + fireEvent.click(getByTestSubject('customAction')); + await waitForEuiPopoverClose(); }); }); From f0b17d9d9687e3969e52e28f1c2e9a1b0acc5e6f Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Tue, 14 Nov 2023 01:33:38 -0800 Subject: [PATCH 5/8] [cleanup] Remove totally unnecessary focus/blur JS logic on custom actions - it's literally not doing anything - there's no hover or opacity behavior at this point --- .../custom_item_action.test.tsx.snap | 5 +- src/components/basic_table/action_types.ts | 2 +- .../basic_table/collapsed_item_actions.tsx | 44 ++------------- .../basic_table/custom_item_action.tsx | 54 +------------------ 4 files changed, 7 insertions(+), 98 deletions(-) diff --git a/src/components/basic_table/__snapshots__/custom_item_action.test.tsx.snap b/src/components/basic_table/__snapshots__/custom_item_action.test.tsx.snap index 578fe7defdf..f8b1953b4e4 100644 --- a/src/components/basic_table/__snapshots__/custom_item_action.test.tsx.snap +++ b/src/components/basic_table/__snapshots__/custom_item_action.test.tsx.snap @@ -4,10 +4,7 @@ exports[`CustomItemAction render 1`] = `
- + test
diff --git a/src/components/basic_table/action_types.ts b/src/components/basic_table/action_types.ts index cc527d97930..09fb84d0109 100644 --- a/src/components/basic_table/action_types.ts +++ b/src/components/basic_table/action_types.ts @@ -72,7 +72,7 @@ export type DefaultItemAction = ExclusiveUnion< export interface CustomItemAction { /** - * The function that renders the action. Note that the returned node is expected to have `onFocus` and `onBlur` functions + * Allows rendering a totally custom action */ render: (item: T, enabled: boolean) => ReactElement; /** diff --git a/src/components/basic_table/collapsed_item_actions.tsx b/src/components/basic_table/collapsed_item_actions.tsx index 5039073a127..74266116866 100644 --- a/src/components/basic_table/collapsed_item_actions.tsx +++ b/src/components/basic_table/collapsed_item_actions.tsx @@ -6,7 +6,7 @@ * Side Public License, v 1. */ -import React, { Component, FocusEvent, ReactNode, ReactElement } from 'react'; +import React, { Component, ReactNode, ReactElement } from 'react'; import { isString } from '../../services/predicate'; import { EuiContextMenuItem, EuiContextMenuPanel } from '../context_menu'; import { EuiPopover } from '../popover'; @@ -22,8 +22,6 @@ export interface CollapsedItemActionsProps { itemId: ItemIdResolved; actionEnabled: (action: Action) => boolean; className?: string; - onFocus?: (event: FocusEvent) => void; - onBlur?: () => void; } interface CollapsedItemActionsState { @@ -40,8 +38,6 @@ export class CollapsedItemActions extends Component< CollapsedItemActionsProps, CollapsedItemActionsState > { - private popoverDiv: HTMLDivElement | null = null; - state = { popoverOpen: false }; togglePopover = () => { @@ -52,45 +48,13 @@ export class CollapsedItemActions extends Component< this.setState({ popoverOpen: false }); }; - onPopoverBlur = () => { - // you must be asking... WTF? I know... but this timeout is - // required to make sure we process the onBlur events after the initial - // event cycle. Reference: - // https://medium.com/@jessebeach/dealing-with-focus-and-blur-in-a-composite-widget-in-react-90d3c3b49a9b - window.requestAnimationFrame(() => { - if ( - !this.popoverDiv!.contains(document.activeElement) && - this.props.onBlur - ) { - this.props.onBlur(); - } - }); - }; - - registerPopoverDiv = (popoverDiv: HTMLDivElement | null) => { - if (!this.popoverDiv) { - this.popoverDiv = popoverDiv; - this.popoverDiv && - this.popoverDiv.addEventListener('focusout', this.onPopoverBlur); - } - }; - - componentWillUnmount() { - if (this.popoverDiv) { - this.popoverDiv.removeEventListener('focusout', this.onPopoverBlur); - } - } - onClickItem = (onClickAction?: () => void) => { this.closePopover(); - if (onClickAction) { - onClickAction(); - } + onClickAction?.(); }; render() { - const { actions, itemId, item, actionEnabled, onFocus, className } = - this.props; + const { actions, itemId, item, actionEnabled, className } = this.props; const isOpen = this.state.popoverOpen; @@ -166,7 +130,6 @@ export class CollapsedItemActions extends Component< color="text" isDisabled={allDisabled} onClick={this.togglePopover.bind(this)} - onFocus={onFocus} data-test-subj="euiCollapsedItemActionsButton" /> )} @@ -186,7 +149,6 @@ export class CollapsedItemActions extends Component< return ( { @@ -25,58 +25,8 @@ export class CustomItemAction extends Component< CustomItemActionProps, CustomItemActionState > { - private mounted: boolean; - - constructor(props: CustomItemActionProps) { - super(props); - this.state = { hasFocus: false }; - - // while generally considered an anti-pattern, here we require - // to do that as the onFocus/onBlur events of the action controls - // may trigger while this component is unmounted. An alternative - // (at least the workarounds suggested by react is to unregister - // the onFocus/onBlur listeners from the action controls... this - // unfortunately will lead to unnecessarily complex code... so we'll - // stick to this approach for now) - this.mounted = false; - } - - componentDidMount() { - this.mounted = true; - } - - componentWillUnmount() { - this.mounted = false; - } - - onFocus = () => { - if (this.mounted) { - this.setState({ hasFocus: true }); - } - }; - - onBlur = () => { - if (this.mounted) { - this.setState({ hasFocus: false }); - } - }; - - hasFocus = () => { - return this.state.hasFocus; - }; - render() { const { action, enabled, item, className } = this.props; - const tool = action.render(item, enabled); - const clonedTool = cloneElement(tool, { - onFocus: this.onFocus, - onBlur: this.onBlur, - }); - const style = this.hasFocus() ? { opacity: 1 } : undefined; - return ( -
- {clonedTool} -
- ); + return
{action.render(item, enabled)}
; } } From a5ea2542f7fdd20c72fa59e6a024c547a5966c3b Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Tue, 14 Nov 2023 01:56:45 -0800 Subject: [PATCH 6/8] [optional][tech debt] Convert from class components to function components - now that all the onFocus/onBlur logic is gone, these components can more easily be function components + fix typescript complaints --- .../basic_table/collapsed_item_actions.tsx | 265 +++++++++--------- .../basic_table/custom_item_action.tsx | 23 +- 2 files changed, 136 insertions(+), 152 deletions(-) diff --git a/src/components/basic_table/collapsed_item_actions.tsx b/src/components/basic_table/collapsed_item_actions.tsx index 74266116866..bc93d2ce296 100644 --- a/src/components/basic_table/collapsed_item_actions.tsx +++ b/src/components/basic_table/collapsed_item_actions.tsx @@ -6,17 +6,25 @@ * Side Public License, v 1. */ -import React, { Component, ReactNode, ReactElement } from 'react'; +import React, { + useState, + useCallback, + useMemo, + ReactNode, + ReactElement, +} from 'react'; + import { isString } from '../../services/predicate'; import { EuiContextMenuItem, EuiContextMenuPanel } from '../context_menu'; import { EuiPopover } from '../popover'; import { EuiButtonIcon } from '../button'; import { EuiToolTip } from '../tool_tip'; import { EuiI18n } from '../i18n'; + import { Action, CustomItemAction } from './action_types'; import { ItemIdResolved } from './table_types'; -export interface CollapsedItemActionsProps { +export interface CollapsedItemActionsProps { actions: Array>; item: T; itemId: ItemIdResolved; @@ -24,143 +32,124 @@ export interface CollapsedItemActionsProps { className?: string; } -interface CollapsedItemActionsState { - popoverOpen: boolean; -} - -function actionIsCustomItemAction( +const actionIsCustomItemAction = ( action: Action -): action is CustomItemAction { - return action.hasOwnProperty('render'); -} - -export class CollapsedItemActions extends Component< - CollapsedItemActionsProps, - CollapsedItemActionsState -> { - state = { popoverOpen: false }; - - togglePopover = () => { - this.setState((prevState) => ({ popoverOpen: !prevState.popoverOpen })); - }; - - closePopover = () => { - this.setState({ popoverOpen: false }); - }; - - onClickItem = (onClickAction?: () => void) => { - this.closePopover(); +): action is CustomItemAction => action.hasOwnProperty('render'); + +export const CollapsedItemActions = ({ + actions, + itemId, + item, + actionEnabled, + className, +}: CollapsedItemActionsProps) => { + const [popoverOpen, setPopoverOpen] = useState(false); + const [allDisabled, setAllDisabled] = useState(true); + + const onClickItem = useCallback((onClickAction?: () => void) => { + setPopoverOpen(false); onClickAction?.(); - }; - - render() { - const { actions, itemId, item, actionEnabled, className } = this.props; - - const isOpen = this.state.popoverOpen; - - let allDisabled = true; - const controls = actions.reduce( - (controls, action, index) => { - const key = `action_${itemId}_${index}`; - const available = action.available ? action.available(item) : true; - if (!available) { - return controls; - } - const enabled = actionEnabled(action); - allDisabled = allDisabled && !enabled; - if (actionIsCustomItemAction(action)) { - const customAction = action as CustomItemAction; - const actionControl = customAction.render(item, enabled); - controls.push( - // Do not put the `onClick` on the EuiContextMenuItem itself - otherwise - // it renders a