Add code review suggestions [2]

elastic · Dec 19, 2022 · cf8373e · cf8373e
1 parent cd96c5e
commit cf8373e
Show file tree

Hide file tree

Showing 9 changed files with 97 additions and 55 deletions.
diff --git a/src/plugins/ui_actions/public/cell_actions/components/cell_actions.stories.tsx b/src/plugins/ui_actions/public/cell_actions/components/cell_actions.stories.tsx
@@ -57,11 +57,11 @@ DefaultWithControls.argTypes = {
 };
 
 DefaultWithControls.args = {
-  showTooltip: true,
+  showActionTooltips: true,
   mode: CellActionsMode.ALWAYS_VISIBLE,
   triggerId: TRIGGER_ID,
   field: FIELD,
-  showMoreActionsFrom: 3,
+  visibleCellActions: 3,
 };
 
 export const CellActionInline = ({}: {}) => (

diff --git a/src/plugins/ui_actions/public/cell_actions/components/cell_actions.tsx b/src/plugins/ui_actions/public/cell_actions/components/cell_actions.tsx
@@ -70,11 +70,15 @@ export interface CellActionsProps {
    * `ALWAYS_VISIBLE` always shows the actions.
    */
   mode: CellActionsMode;
-  showTooltip?: boolean;
+
+  /**
+   * It displays a tooltip for every action button when `true`.
+   */
+  showActionTooltips?: boolean;
   /**
    * It shows 'more actions' button when the number of actions is bigger than this parameter.
    */
-  showMoreActionsFrom?: number;
+  visibleCellActions?: number;
   /**
    * Custom set of properties used by some actions.
    * An action might require a specific set of metadata properties to render.
@@ -88,8 +92,8 @@ export const CellActions: React.FC<CellActionsProps> = ({
   triggerId,
   children,
   mode,
-  showTooltip = true,
-  showMoreActionsFrom = 3,
+  showActionTooltips = true,
+  visibleCellActions = 3,
   metadata,
 }) => {
   const extraContentNodeRef = useRef<HTMLDivElement | null>(null);
@@ -111,8 +115,8 @@ export const CellActions: React.FC<CellActionsProps> = ({
       <div ref={nodeRef} data-test-subj={'cellActions'}>
         <HoverActionsPopover
           actionContext={actionContext}
-          showTooltip={showTooltip}
-          showMoreActionsFrom={showMoreActionsFrom}
+          showActionTooltips={showActionTooltips}
+          visibleCellActions={visibleCellActions}
         >
           {children}
         </HoverActionsPopover>
@@ -127,8 +131,8 @@ export const CellActions: React.FC<CellActionsProps> = ({
       {children}
       <InlineActions
         actionContext={actionContext}
-        showTooltip={showTooltip}
-        showMoreActionsFrom={showMoreActionsFrom}
+        showActionTooltips={showActionTooltips}
+        visibleCellActions={visibleCellActions}
       />
       <div ref={extraContentNodeRef} />
     </div>

diff --git a/src/plugins/ui_actions/public/cell_actions/components/cell_actions_context.test.tsx b/src/plugins/ui_actions/public/cell_actions/components/cell_actions_context.test.tsx
@@ -75,7 +75,7 @@ describe('CellActionsContextProvider', () => {
     expect(result.current.value).toEqual([action]);
   });
 
-  it('sorts actions', async () => {
+  it('sorts actions by order', async () => {
     const firstAction = makeAction('action-1', 'icon', 1);
     const secondAction = makeAction('action-2', 'icon', 2);
     const getActionsPromise = Promise.resolve([secondAction, firstAction]);
@@ -125,4 +125,31 @@ describe('CellActionsContextProvider', () => {
 
     expect(result.current.value).toEqual([firstAction, secondAction]);
   });
+
+  it('sorts actions by id and order', async () => {
+    const actionWithoutOrder = makeAction('action-1-no-order');
+    const secondAction = makeAction('action-2', 'icon', 2);
+    const thirdAction = makeAction('action-3', 'icon', 3);
+
+    const getActionsPromise = Promise.resolve([secondAction, actionWithoutOrder, thirdAction]);
+    const getActions = () => getActionsPromise;
+
+    const { result } = renderHook(
+      () => useLoadActions(actionContext),
+
+      {
+        wrapper: ({ children }) => (
+          <CellActionsContextProvider getTriggerCompatibleActions={getActions}>
+            {children}
+          </CellActionsContextProvider>
+        ),
+      }
+    );
+
+    await act(async () => {
+      await getActionsPromise;
+    });
+
+    expect(result.current.value).toEqual([secondAction, thirdAction, actionWithoutOrder]);
+  });
 });
diff --git a/src/plugins/ui_actions/public/cell_actions/components/hover_actions_popover.test.tsx b/src/plugins/ui_actions/public/cell_actions/components/hover_actions_popover.test.tsx
@@ -27,9 +27,9 @@ describe('HoverActionsPopover', () => {
       <CellActionsContextProvider getTriggerCompatibleActions={getActions}>
         <HoverActionsPopover
           children={null}
-          showMoreActionsFrom={4}
+          visibleCellActions={4}
           actionContext={actionContext}
-          showTooltip={false}
+          showActionTooltips={false}
         />
       </CellActionsContextProvider>
     );
@@ -44,9 +44,9 @@ describe('HoverActionsPopover', () => {
     const { queryByLabelText, getByTestId } = render(
       <CellActionsContextProvider getTriggerCompatibleActions={getActions}>
         <HoverActionsPopover
-          showMoreActionsFrom={4}
+          visibleCellActions={4}
           actionContext={actionContext}
-          showTooltip={false}
+          showActionTooltips={false}
         >
           <TestComponent />
         </HoverActionsPopover>
@@ -69,9 +69,9 @@ describe('HoverActionsPopover', () => {
     const { queryByLabelText, getByTestId } = render(
       <CellActionsContextProvider getTriggerCompatibleActions={getActions}>
         <HoverActionsPopover
-          showMoreActionsFrom={4}
+          visibleCellActions={4}
           actionContext={actionContext}
-          showTooltip={false}
+          showActionTooltips={false}
         >
           <TestComponent />
         </HoverActionsPopover>
@@ -99,9 +99,9 @@ describe('HoverActionsPopover', () => {
     const { getByTestId } = render(
       <CellActionsContextProvider getTriggerCompatibleActions={getActions}>
         <HoverActionsPopover
-          showMoreActionsFrom={1}
+          visibleCellActions={1}
           actionContext={actionContext}
-          showTooltip={false}
+          showActionTooltips={false}
         >
           <TestComponent />
         </HoverActionsPopover>
@@ -124,9 +124,9 @@ describe('HoverActionsPopover', () => {
     const { getByTestId, getByLabelText } = render(
       <CellActionsContextProvider getTriggerCompatibleActions={getActions}>
         <HoverActionsPopover
-          showMoreActionsFrom={1}
+          visibleCellActions={1}
           actionContext={actionContext}
-          showTooltip={false}
+          showActionTooltips={false}
         >
           <TestComponent />
         </HoverActionsPopover>
@@ -158,9 +158,9 @@ describe('HoverActionsPopover', () => {
     const { getByTestId, queryByLabelText } = render(
       <CellActionsContextProvider getTriggerCompatibleActions={getActions}>
         <HoverActionsPopover
-          showMoreActionsFrom={2}
+          visibleCellActions={2}
           actionContext={actionContext}
-          showTooltip={false}
+          showActionTooltips={false}
         >
           <TestComponent />
         </HoverActionsPopover>

diff --git a/src/plugins/ui_actions/public/cell_actions/components/hover_actions_popover.tsx b/src/plugins/ui_actions/public/cell_actions/components/hover_actions_popover.tsx
@@ -37,13 +37,13 @@ const HOVER_INTENT_DELAY = 100; // ms
 
 interface Props {
   children: React.ReactNode;
-  showMoreActionsFrom: number;
+  visibleCellActions: number;
   actionContext: CellActionExecutionContext;
-  showTooltip: boolean;
+  showActionTooltips: boolean;
 }
 
 export const HoverActionsPopover = React.memo<Props>(
-  ({ children, showMoreActionsFrom, actionContext, showTooltip }) => {
+  ({ children, visibleCellActions, actionContext, showActionTooltips }) => {
     const contentRef = useRef<HTMLDivElement>(null);
     const [isExtraActionsPopoverOpen, setIsExtraActionsPopoverOpen] = useState(false);
     const [showHoverContent, setShowHoverContent] = useState(false);
@@ -52,8 +52,8 @@ export const HoverActionsPopover = React.memo<Props>(
     const [{ value: actions }, loadActions] = useLoadActionsFn();
 
     const { visibleActions, extraActions } = useMemo(
-      () => partitionActions(actions ?? [], showMoreActionsFrom),
-      [actions, showMoreActionsFrom]
+      () => partitionActions(actions ?? [], visibleCellActions),
+      [actions, visibleCellActions]
     );
 
     const closePopover = useCallback(() => {
@@ -139,11 +139,14 @@ export const HoverActionsPopover = React.memo<Props>(
                     key={action.id}
                     action={action}
                     actionContext={actionContext}
-                    showTooltip={showTooltip}
+                    showTooltip={showActionTooltips}
                   />
                 ))}
                 {extraActions.length > 0 ? (
-                  <ExtraActionsButton onClick={onShowExtraActionsClick} showTooltip={showTooltip} />
+                  <ExtraActionsButton
+                    onClick={onShowExtraActionsClick}
+                    showTooltip={showActionTooltips}
+                  />
                 ) : null}
               </div>
             ) : null}

diff --git a/src/plugins/ui_actions/public/cell_actions/components/inline_actions.test.tsx b/src/plugins/ui_actions/public/cell_actions/components/inline_actions.test.tsx
@@ -20,7 +20,11 @@ describe('InlineActions', () => {
     const getActions = () => getActionsPromise;
     const { queryByTestId } = render(
       <CellActionsContextProvider getTriggerCompatibleActions={getActions}>
-        <InlineActions showMoreActionsFrom={5} actionContext={actionContext} showTooltip={false} />
+        <InlineActions
+          visibleCellActions={5}
+          actionContext={actionContext}
+          showActionTooltips={false}
+        />
       </CellActionsContextProvider>
     );
 
@@ -42,7 +46,11 @@ describe('InlineActions', () => {
     const getActions = () => getActionsPromise;
     const { queryAllByRole } = render(
       <CellActionsContextProvider getTriggerCompatibleActions={getActions}>
-        <InlineActions showMoreActionsFrom={5} actionContext={actionContext} showTooltip={false} />
+        <InlineActions
+          visibleCellActions={5}
+          actionContext={actionContext}
+          showActionTooltips={false}
+        />
       </CellActionsContextProvider>
     );
 

diff --git a/src/plugins/ui_actions/public/cell_actions/components/inline_actions.tsx b/src/plugins/ui_actions/public/cell_actions/components/inline_actions.tsx
@@ -16,26 +16,26 @@ import { useLoadActions } from './cell_actions_context';
 
 interface InlineActionsProps {
   actionContext: CellActionExecutionContext;
-  showTooltip: boolean;
-  showMoreActionsFrom: number;
+  showActionTooltips: boolean;
+  visibleCellActions: number;
 }
 
 export const InlineActions: React.FC<InlineActionsProps> = ({
   actionContext,
-  showTooltip,
-  showMoreActionsFrom,
+  showActionTooltips,
+  visibleCellActions,
 }) => {
   const { value: allActions } = useLoadActions(actionContext);
   const { extraActions, visibleActions } = usePartitionActions(
     allActions ?? [],
-    showMoreActionsFrom
+    visibleCellActions
   );
   const [isPopoverOpen, setIsPopoverOpen] = useState(false);
   const togglePopOver = useCallback(() => setIsPopoverOpen((isOpen) => !isOpen), []);
   const closePopOver = useCallback(() => setIsPopoverOpen(false), []);
   const button = useMemo(
-    () => <ExtraActionsButton onClick={togglePopOver} showTooltip={showTooltip} />,
-    [togglePopOver, showTooltip]
+    () => <ExtraActionsButton onClick={togglePopOver} showTooltip={showActionTooltips} />,
+    [togglePopOver, showActionTooltips]
   );
 
   return (
@@ -45,7 +45,7 @@ export const InlineActions: React.FC<InlineActionsProps> = ({
           key={`action-item-${index}`}
           action={action}
           actionContext={actionContext}
-          showTooltip={showTooltip}
+          showTooltip={showActionTooltips}
         />
       ))}
       {extraActions.length > 0 ? (

diff --git a/src/plugins/ui_actions/public/cell_actions/hooks/actions.test.ts b/src/plugins/ui_actions/public/cell_actions/hooks/actions.test.ts
@@ -17,39 +17,39 @@ describe('InlineActions', () => {
     expect(extraActions).toEqual([]);
   });
 
-  it('returns only visible actions when showMoreActionsFrom > actions.length', async () => {
+  it('returns only visible actions when visibleCellActions > actions.length', async () => {
     const actions = [makeAction('action-1'), makeAction('action-2'), makeAction('action-3')];
     const { extraActions, visibleActions } = partitionActions(actions, 4);
 
     expect(visibleActions.length).toEqual(actions.length);
     expect(extraActions).toEqual([]);
   });
 
-  it('returns only extra actions when showMoreActionsFrom is 1', async () => {
+  it('returns only extra actions when visibleCellActions is 1', async () => {
     const actions = [makeAction('action-1'), makeAction('action-2'), makeAction('action-3')];
     const { extraActions, visibleActions } = partitionActions(actions, 1);
 
     expect(visibleActions).toEqual([]);
     expect(extraActions.length).toEqual(actions.length);
   });
 
-  it('returns only extra actions when showMoreActionsFrom is 0', async () => {
+  it('returns only extra actions when visibleCellActions is 0', async () => {
     const actions = [makeAction('action-1'), makeAction('action-2'), makeAction('action-3')];
     const { extraActions, visibleActions } = partitionActions(actions, 0);
 
     expect(visibleActions).toEqual([]);
     expect(extraActions.length).toEqual(actions.length);
   });
 
-  it('returns only extra actions when showMoreActionsFrom is negative', async () => {
+  it('returns only extra actions when visibleCellActions is negative', async () => {
     const actions = [makeAction('action-1'), makeAction('action-2'), makeAction('action-3')];
     const { extraActions, visibleActions } = partitionActions(actions, -6);
 
     expect(visibleActions).toEqual([]);
     expect(extraActions.length).toEqual(actions.length);
   });
 
-  it('returns only one visible action when showMoreActionsFrom is 2 and action.length is 3', async () => {
+  it('returns only one visible action when visibleCellActionss 2 and action.length is 3', async () => {
     const { extraActions, visibleActions } = partitionActions(
       [makeAction('action-1'), makeAction('action-2'), makeAction('action-3')],
       2
@@ -59,7 +59,7 @@ describe('InlineActions', () => {
     expect(extraActions.length).toEqual(2);
   });
 
-  it('returns two visible actions when showMoreActionsFrom is 3 and action.length is 5', async () => {
+  it('returns two visible actions when visibleCellActions is 3 and action.length is 5', async () => {
     const { extraActions, visibleActions } = partitionActions(
       [
         makeAction('action-1'),
@@ -74,7 +74,7 @@ describe('InlineActions', () => {
     expect(extraActions.length).toEqual(3);
   });
 
-  it('returns three visible actions when showMoreActionsFrom is 3 and action.length is 3', async () => {
+  it('returns three visible actions when visibleCellActions is 3 and action.length is 3', async () => {
     const { extraActions, visibleActions } = partitionActions(
       [makeAction('action-1'), makeAction('action-2'), makeAction('action-3')],
       3

diff --git a/src/plugins/ui_actions/public/cell_actions/hooks/actions.ts b/src/plugins/ui_actions/public/cell_actions/hooks/actions.ts
@@ -9,13 +9,13 @@
 import { useMemo } from 'react';
 import { Action } from '../../actions';
 
-export const partitionActions = (actions: Action[], showMoreActionsFrom: number) => {
-  if (showMoreActionsFrom <= 1) return { extraActions: actions, visibleActions: [] };
-  if (actions.length <= showMoreActionsFrom) return { extraActions: [], visibleActions: actions };
+export const partitionActions = (actions: Action[], visibleCellActions: number) => {
+  if (visibleCellActions <= 1) return { extraActions: actions, visibleActions: [] };
+  if (actions.length <= visibleCellActions) return { extraActions: [], visibleActions: actions };
 
   return {
-    visibleActions: actions.slice(0, showMoreActionsFrom - 1),
-    extraActions: actions.slice(showMoreActionsFrom - 1, actions.length),
+    visibleActions: actions.slice(0, visibleCellActions - 1),
+    extraActions: actions.slice(visibleCellActions - 1, actions.length),
   };
 };
 
@@ -26,9 +26,9 @@ export interface PartitionedActions {
 
 export const usePartitionActions = (
   allActions: Action[],
-  showMoreActionsFrom: number
+  visibleCellActions: number
 ): PartitionedActions => {
   return useMemo(() => {
-    return partitionActions(allActions ?? [], showMoreActionsFrom);
-  }, [allActions, showMoreActionsFrom]);
+    return partitionActions(allActions ?? [], visibleCellActions);
+  }, [allActions, visibleCellActions]);
 };