Skip to content

Commit

Permalink
[Actions] Tweak the way the action menu gets calculated (#11497)
Browse files Browse the repository at this point in the history
### WHY are these changes introduced?

The way that the Actions within the ActionMenu get populated currently
mean that on the initial render, we render no buttons at all as we are
waiting for a calculation to occur. This works fine in the real world,
but causes issues for tests which expect the presence of buttons. This
PR attempts to ensure our components are more predictable in test cases
and also robust in real world examples.


### WHAT is this pull request doing?

The ActionsMeasurer component used a `requestAnimationFrame` within the
`useCallback` to calculate the values needed to determine which actions
to show and which to roll up. This is an unnecessary code complexity
that can be simplified to just run inside the `useCallback`.

We were finding that using the `requestAnimationFrame` here was causing
extra event loop ticks to be created in test suites and creating side
effects and adding no benefit whatsoever, so removing it is a
no-brainer.

### How to 🎩

🖥 [Local development
instructions](https://github.com/Shopify/polaris/blob/main/README.md#install-dependencies-and-build-workspaces)
🗒 [General tophatting
guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md)
📄 [Changelog
guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog)

Spin URL:
https://admin.web.bulk-ui-3.marc-thomas.eu.spin.dev/store/shop1/products/13


### 🎩 checklist

- [ ] Tested a
[snapshot](https://github.com/Shopify/polaris/blob/main/documentation/Releasing.md#-snapshot-releases)
- [x] Tested on
[mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing)
- [x] Tested on [multiple
browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers)
- [x] Tested for
[accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md)
- [x] Updated the component's `README.md` with documentation changes
- [x] [Tophatted
documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md)
changes in the style guide

---------

Co-authored-by: Chloe Rice <chloerice@users.noreply.github.com>
  • Loading branch information
mrcthms and chloerice authored Jan 24, 2024
1 parent ac004fc commit d50cc6d
Show file tree
Hide file tree
Showing 7 changed files with 154 additions and 65 deletions.
5 changes: 5 additions & 0 deletions .changeset/fifty-kids-begin.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/polaris': minor
---

Improved test reliability for non-rolled up actions in `ActionMenu`
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@
}
}

.ActionsLayout--measuring {
visibility: hidden;
height: 0;
}

.ActionsLayoutMeasurer {
display: flex;
flex-wrap: wrap;
Expand Down
142 changes: 110 additions & 32 deletions polaris-react/src/components/ActionMenu/components/Actions/Actions.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
import React, {useCallback, useRef, useState} from 'react';
import React, {
useCallback,
useRef,
useState,
useReducer,
useEffect,
useMemo,
} from 'react';

import type {
ActionListItemDescriptor,
Expand All @@ -9,6 +16,7 @@ import type {
import {MenuGroup} from '../MenuGroup';
import {useI18n} from '../../../../utilities/i18n';
import {SecondaryAction} from '../SecondaryAction';
import {classNames} from '../../../../utilities/css';

import styles from './Actions.module.scss';
import type {ActionsMeasurements} from './components';
Expand All @@ -23,28 +31,50 @@ interface Props {
/** Callback that returns true when secondary actions are rolled up into action groups, and false when not */
onActionRollup?(hasRolledUp: boolean): void;
}
interface MeasuredActionsIndices {
interface ActionsState {
visibleActions: number[];
hiddenActions: number[];
visibleGroups: number[];
hiddenGroups: number[];
actionsWidths: number[];
containerWidth: number;
disclosureWidth: number;
hasMeasured: boolean;
}

export function Actions({actions = [], groups = [], onActionRollup}: Props) {
export function Actions({actions, groups, onActionRollup}: Props) {
const i18n = useI18n();
const rollupActiveRef = useRef<boolean | null>(null);
const [activeMenuGroup, setActiveMenuGroup] = useState<string | undefined>(
undefined,
);
const [
{visibleActions, hiddenActions, visibleGroups, hiddenGroups},
setMeasuredActionsIndices,
] = useState<MeasuredActionsIndices>({
visibleActions: [],
hiddenActions: [],
visibleGroups: [],
hiddenGroups: [],
});

const [state, setState] = useReducer(
(data: ActionsState, partialData: Partial<ActionsState>): ActionsState => {
return {...data, ...partialData};
},
{
disclosureWidth: 0,
containerWidth: Infinity,
actionsWidths: [],
visibleActions: [],
hiddenActions: [],
visibleGroups: [],
hiddenGroups: [],
hasMeasured: false,
},
);

const {
visibleActions,
hiddenActions,
visibleGroups,
hiddenGroups,
containerWidth,
disclosureWidth,
actionsWidths,
hasMeasured,
} = state;

const defaultRollupGroup: MenuGroupDescriptor = {
title: i18n.translate('Polaris.ActionMenu.Actions.moreActions'),
Expand All @@ -61,27 +91,62 @@ export function Actions({actions = [], groups = [], onActionRollup}: Props) {
[],
);

const actionsMarkup = actions.map((action, index) => {
if (!visibleActions.includes(index)) {
return null;
useEffect(() => {
if (containerWidth === 0) {
return;
}
const {visibleActions, visibleGroups, hiddenActions, hiddenGroups} =
getVisibleAndHiddenActionsIndices(
actions,
groups,
disclosureWidth,
actionsWidths,
containerWidth,
);
setState({
visibleActions,
visibleGroups,
hiddenActions,
hiddenGroups,
hasMeasured: containerWidth !== Infinity,
});
}, [
containerWidth,
disclosureWidth,
actions,
groups,
actionsWidths,
setState,
]);

const {content, onAction, ...rest} = action;
const actionsOrDefault = useMemo(() => actions ?? [], [actions]);
const groupsOrDefault = useMemo(() => groups ?? [], [groups]);

return (
<SecondaryAction key={content} onClick={onAction} {...rest}>
{content}
</SecondaryAction>
);
});
const actionsMarkup = actionsOrDefault
.filter((_, index) => {
if (!visibleActions.includes(index)) {
return false;
}

return true;
})
.map((action) => {
const {content, onAction, ...rest} = action;

return (
<SecondaryAction key={content} onClick={onAction} {...rest}>
{content}
</SecondaryAction>
);
});

const groupsToFilter =
hiddenGroups.length > 0 || hiddenActions.length > 0
? [...groups, defaultRollupGroup]
: [...groups];
? [...groupsOrDefault, defaultRollupGroup]
: [...groupsOrDefault];

const filteredGroups = groupsToFilter.filter((group, index) => {
const hasNoGroupsProp = groups.length === 0;
const hasNoGroupsProp = groupsOrDefault.length === 0;
const isVisibleGroup = visibleGroups.includes(index);
const isDefaultGroup = group === defaultRollupGroup;

Expand All @@ -96,8 +161,12 @@ export function Actions({actions = [], groups = [], onActionRollup}: Props) {
return isVisibleGroup;
});

const hiddenActionObjects = hiddenActions.map((index) => actions[index]);
const hiddenGroupObjects = hiddenGroups.map((index) => groups[index]);
const hiddenActionObjects = hiddenActions.map(
(index) => actionsOrDefault[index],
);
const hiddenGroupObjects = hiddenGroups.map(
(index) => groupsOrDefault[index],
);

const groupsMarkup = filteredGroups.map((group) => {
const {title, actions: groupActions, ...rest} = group;
Expand Down Expand Up @@ -160,8 +229,8 @@ export function Actions({actions = [], groups = [], onActionRollup}: Props) {

const {visibleActions, hiddenActions, visibleGroups, hiddenGroups} =
getVisibleAndHiddenActionsIndices(
actions,
groups,
actionsOrDefault,
groupsOrDefault,
disclosureWidth,
actionsWidths,
containerWidth,
Expand All @@ -175,14 +244,18 @@ export function Actions({actions = [], groups = [], onActionRollup}: Props) {
rollupActiveRef.current = isRollupActive;
}
}
setMeasuredActionsIndices({
setState({
visibleActions,
hiddenActions,
visibleGroups,
hiddenGroups,
actionsWidths,
containerWidth,
disclosureWidth,
hasMeasured: true,
});
},
[actions, groups, onActionRollup],
[actionsOrDefault, groupsOrDefault, onActionRollup],
);

const actionsMeasurer = (
Expand All @@ -196,7 +269,12 @@ export function Actions({actions = [], groups = [], onActionRollup}: Props) {
return (
<div className={styles.ActionsLayoutOuter}>
{actionsMeasurer}
<div className={styles.ActionsLayout}>
<div
className={classNames(
styles.ActionsLayout,
!hasMeasured && styles['ActionsLayout--measuring'],
)}
>
{actionsMarkup}
{groupsMarkup}
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import type {
MenuActionDescriptor,
MenuGroupDescriptor,
} from '../../../../../../types';
import {useComponentDidMount} from '../../../../../../utilities/use-component-did-mount';
import {useI18n} from '../../../../../../utilities/i18n';
import {SecondaryAction} from '../../../SecondaryAction';
import {useEventListener} from '../../../../../../utilities/use-event-listener';
Expand Down Expand Up @@ -33,7 +32,6 @@ export function ActionsMeasurer({
}: ActionsMeasurerProps) {
const i18n = useI18n();
const containerNode = useRef<HTMLDivElement>(null);
const animationFrame = useRef<number | null>(null);

const defaultRollupGroup: MenuGroupDescriptor = {
title: i18n.translate('Polaris.ActionMenu.Actions.moreActions'),
Expand All @@ -45,42 +43,30 @@ export function ActionsMeasurer({
);

const handleMeasurement = useCallback(() => {
if (animationFrame.current) {
cancelAnimationFrame(animationFrame.current);
if (!containerNode.current) {
return;
}

animationFrame.current = requestAnimationFrame(() => {
if (!containerNode.current) {
return;
}

const containerWidth = containerNode.current.offsetWidth;
const hiddenActionNodes = containerNode.current.children;
const hiddenActionNodesArray = Array.from(hiddenActionNodes);
const hiddenActionsWidths = hiddenActionNodesArray.map((node) => {
const buttonWidth = Math.ceil(node.getBoundingClientRect().width);
return buttonWidth + ACTION_SPACING;
});
const disclosureWidth = hiddenActionsWidths.pop() || 0;

handleMeasurementProp({
containerWidth,
disclosureWidth,
hiddenActionsWidths,
});
const containerWidth = containerNode.current.offsetWidth;
const hiddenActionNodes = containerNode.current.children;
const hiddenActionNodesArray = Array.from(hiddenActionNodes);
const hiddenActionsWidths = hiddenActionNodesArray.map((node) => {
const buttonWidth = Math.ceil(node.getBoundingClientRect().width);
return buttonWidth + ACTION_SPACING;
});
const disclosureWidth = hiddenActionsWidths.pop() || 0;

handleMeasurementProp({
containerWidth,
disclosureWidth,
hiddenActionsWidths,
});
}, [handleMeasurementProp]);

useEffect(() => {
handleMeasurement();
}, [handleMeasurement, actions, groups]);

useComponentDidMount(() => {
if (process.env.NODE_ENV === 'development') {
setTimeout(handleMeasurement, 0);
}
});

const actionsMarkup = actions.map((action) => {
const {content, onAction, ...rest} = action;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {Actions, MenuGroup, RollupActions, SecondaryAction} from '../..';
import {Tooltip} from '../../../../Tooltip';
import type {getVisibleAndHiddenActionsIndices} from '../utilities';
import {ActionsMeasurer} from '../components';
import styles from '../Actions.module.scss';

jest.mock('../components/ActionsMeasurer', () => ({
ActionsMeasurer() {
Expand Down Expand Up @@ -71,9 +72,11 @@ describe('<Actions />', () => {
<ActionMenu actions={actionsBeforeOverriddenOrder} />,
);

const wrappingDiv = findWrapper(wrapper);

forceMeasurement(wrapper);

expect(wrapper.findAll(SecondaryAction)).toHaveLength(3);
expect(wrappingDiv!.findAll(SecondaryAction)).toHaveLength(3);
});

it('renders a <Tooltip /> when helpText is set on an action', () => {
Expand Down Expand Up @@ -233,6 +236,18 @@ describe('<Actions />', () => {
});
});

function findWrapper(wrapper: CustomRoot<any, any>) {
const wrappingDiv = wrapper.findWhere<'div'>((node) => {
return (
node.is('div') &&
Boolean(node.prop('className')) &&
node.prop('className')!.includes(styles.ActionsLayout)
);
});

return wrappingDiv;
}

function forceMeasurement(wrapper: CustomRoot<any, any>) {
wrapper.act(() => {
wrapper.find(ActionsMeasurer)!.trigger('handleMeasurement', {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
export function getVisibleAndHiddenActionsIndices(
actions: any[],
groups: any[],
actions: any[] = [],
groups: any[] = [],
disclosureWidth: number,
actionsWidths: number[],
containerWidth: number,
Expand Down
2 changes: 1 addition & 1 deletion polaris-react/src/components/Page/Page.stories.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, {useState, useCallback} from 'react';
import React from 'react';
import type {ComponentMeta} from '@storybook/react';
import {
DeleteIcon,
Expand Down

0 comments on commit d50cc6d

Please sign in to comment.