refactor(accordions): uplift for react v18

[Francois-Esquire]

Description

Refactoring the Accordion and Stepper for React v18 compatibility.

Detail

This PR updates the two component sets in the accordions family:

• Stepper: replaces the ref for ordinal counting of Stepper.Steps with the StepContext that wraps each Stepper.Step (or child, if wrapped) with a pre-formed context value computed from the current step value - the context contains properties used throughout each Step (and the nested components) and it's current value.
• Accordion: replaces the ref for ordinal counting of Accordion.Sections with the SectionContext which will wrap the Accordion.Section child (or any wrapping element). The SectionContext contains the section value as the context value as it was prior. There was an uplift of @zendeskgarden/container-accordion to v3 and adopted the new API for useAccordion as well.

Fixes #1462

Checklist

• 👌 design updates will be Garden Designer approved (add the designer as a reviewer)
• 🌐 demo is up-to-date (yarn start)
• ⬅️ renders as expected with reversed (RTL) direction
• 🤘 renders as expected with Bedrock CSS (?bedrock)
• 💂‍♂️ includes new unit tests. Maintain existing coverage (always >= 96%)
• ♿ tested for WCAG 2.1 AA accessibility compliance
• 📝 tested in Chrome, Firefox, Safari, and Edge

[coveralls]

coverage: 95.5% (-0.03%) from 95.531% when pulling 20169f3 on mike/refactor/accordions-react-v18-uplift into cc14751 on main.

[jzempel]

Could you extract const value: IStepContext = { ... }; to the top of this function to get rid of this eslint-disable?

[Francois-Esquire]

Moving the variable doesn't get rid of the eslint-disable rule unfortunately. The rule states that context value should be wrapped in a useMemo to avoid re-renders- being that it is already in a useMemo it's not permitted nor does it give any benefit.

[jzempel]

I think {...getPanelProps() as HTMLAttributes<HTMLElement>} would be preferable over the use of any here.

[jzempel]

prefer {...getHeaderProps() as HTMLAttributes<HTMLDivElement>} vs overuse of any.

[jzempel]

I'd like to hear a little bit more about this children wrapping vs. the previous context setup for this component. Should there be concerns about a more brittle DOM structure that is unable to handle intervening nodes?

Also, if this sticks, can isValidElement be used to eliminate the any casting? Ref:

react-components/packages/dropdowns.next/src/elements/combobox/utils.ts

Lines 38 to 67 in 3b33288

	/**
	* Convert an array of `Option` and `OptGroup` children to a valid `options`
	* data structure for `useCombobox` (collect `tagProps` along the way).
	*
	* @param children The `children` prop from `Combobox`.
	* @param optionTagProps A collection that maps option values to tag props.
	*
	* @returns A valid `IUseComboboxProps['options']` data structure.
	*/
	export const toOptions = (
	children: ReactNode,
	optionTagProps: Record<string, IOptionProps['tagProps']>
	) =>
	Children.toArray(children).reduce((options: IUseComboboxProps['options'], option) => {
	const retVal = options;
	if (isValidElement(option)) {
	if ('value' in option.props) {
	retVal.push(toOption(option.props));
	optionTagProps[toString(option.props)] = option.props.tagProps;
	} else {
	const props: IOptGroupProps = option.props;
	const groupOptions = toOptions(props.children, optionTagProps) as IOption[];
	retVal.push({ label: props.label, options: groupOptions });
	}
	}
	return retVal;
	}, []);

[Francois-Esquire]

The idea here is that we expect:

<accordion>
  <section />
  <section />
  <div>
    <section />
  </div>
</accordion>

would give each group the context it needs. This of course relies on the children to take this into account. It actually provides more flexibility when it comes to unique DOM structure requirements, so long as the top level children follow the given pattern.

[jzempel]

The prop inclusion plus other spread is over-complicating the TS here. It's important to stick closer to the underlying container definition and not use any. Here's a solution:

      <StyledHeader
        isFocused={isFocused}
        isExpanded={isExpanded}
        isCollapsible={isCollapsible}
        {...getHeaderProps({
          ref,
          'aria-level': ariaLevel,
          onClick: composeEventHandlers(onClick, onTriggerClick),
          onFocus: composeEventHandlers(onFocus, onHeaderFocus),
          onBlur: composeEventHandlers(onBlur, () => setIsFocused(false)),
          onMouseOver: composeEventHandlers(onMouseOver, () => setIsHovered(true)),
          onMouseOut: composeEventHandlers(onMouseOut, () => setIsHovered(false)),
          role: role === undefined || role === null ? role : 'heading',
          ...other
        }) as HTMLAttributes<HTMLDivElement>}
      >

...break role out in the component props above.

[jzempel]

Similar here. Solution is:

    <StyledPanel
      isBare={isBare}
      isCompact={isCompact}
      isExpanded={isExpanded}
      isAnimated={isAnimated}
      {...getPanelProps({
        role: role === undefined ? null : 'region',
        ref: mergeRefs([panelRef, ref]),
        value: sectionValue,
        ...props
      }) as HTMLAttributes<HTMLElement>}
    >

Change const panelRef = useRef<HTMLElement>(null); above and extract role from the component props.

[jzempel]

Suggested change

	({ isHorizontal, activeIndex, children, ...props }, ref) => {
	({ isHorizontal, activeIndex = DEFAULT_ACTIVE_INDEX, children, ...props }, ref) => {

...where const DEFAULT_ACTIVE_INDEX = 0; is defined above and also applied below as...

StepperComponent.defaultProps = {
  activeIndex: DEFAULT_ACTIVE_INDEX
};

[Francois-Esquire]

Updated! About defaultProps, setting a default value for activeIndex serves the same purpose, does it not?

[jzempel]

Suggested change

	activeIndex: activeIndex!
	activeIndex

...with DEFAULT_ACTIVE_INDEX set above.

[jzempel]

This needs to be retained for runtime checking.