From 6883d7d38aa1956c6b32ecef2d6a21ca73eb658c Mon Sep 17 00:00:00 2001 From: Lukas Harbarth Date: Tue, 22 Oct 2024 16:47:37 +0200 Subject: [PATCH 1/6] cherry-pick 053a0f0 --- .../components/Toolbar/OverflowPopover.tsx | 75 ++++++---- .../components/Toolbar/Toolbar.stories.tsx | 135 +++++++++++++----- .../compat/src/components/Toolbar/index.tsx | 13 +- 3 files changed, 149 insertions(+), 74 deletions(-) diff --git a/packages/compat/src/components/Toolbar/OverflowPopover.tsx b/packages/compat/src/components/Toolbar/OverflowPopover.tsx index 2a0bc18f115..02551612525 100644 --- a/packages/compat/src/components/Toolbar/OverflowPopover.tsx +++ b/packages/compat/src/components/Toolbar/OverflowPopover.tsx @@ -9,15 +9,15 @@ import type { ToggleButtonPropTypes } from '@ui5/webcomponents-react'; import { Popover, ToggleButton } from '@ui5/webcomponents-react'; +import { SHOW_MORE, X_OF_Y } from '@ui5/webcomponents-react/dist/i18n/i18n-defaults.js'; import { stopPropagation } from '@ui5/webcomponents-react/dist/internal/stopPropagation.js'; import { getUi5TagWithSuffix } from '@ui5/webcomponents-react/dist/internal/utils.js'; -import { Device, useSyncRef } from '@ui5/webcomponents-react-base'; +import { Device, useI18nBundle, useSyncRef } from '@ui5/webcomponents-react-base'; import { clsx } from 'clsx'; import type { Dispatch, FC, ReactElement, ReactNode, Ref, SetStateAction } from 'react'; -import { cloneElement, useEffect, useRef, useState } from 'react'; +import { isValidElement, cloneElement, useEffect, useRef, useState } from 'react'; import { createPortal } from 'react-dom'; import { getOverflowPopoverContext } from '../../internal/OverflowPopoverContext.js'; -import type { ToolbarSeparatorPropTypes } from '../ToolbarSeparator/index.js'; import type { ToolbarPropTypes } from './index.js'; interface OverflowPopoverProps { @@ -27,7 +27,6 @@ interface OverflowPopoverProps { portalContainer: Element; overflowContentRef: Ref; numberOfAlwaysVisibleItems?: number; - showMoreText: string; overflowPopoverRef?: Ref; overflowButton?: ReactElement | ReactElement; setIsMounted: Dispatch>; @@ -44,7 +43,6 @@ export const OverflowPopover: FC = (props: OverflowPopover portalContainer, overflowContentRef, numberOfAlwaysVisibleItems, - showMoreText, overflowButton, overflowPopoverRef, setIsMounted, @@ -53,6 +51,8 @@ export const OverflowPopover: FC = (props: OverflowPopover const [pressed, setPressed] = useState(false); const toggleBtnRef = useRef(null); const [componentRef, popoverRef] = useSyncRef(overflowPopoverRef); + const i18nBundle = useI18nBundle('@ui5/webcomponents-react'); + const showMoreText = i18nBundle.getText(SHOW_MORE); useEffect(() => { setIsMounted(true); @@ -123,6 +123,47 @@ export const OverflowPopover: FC = (props: OverflowPopover const OverflowPopoverContextProvider = getOverflowPopoverContext().Provider; + let startIndex = null; + const filteredChildrenArray = children + .map((item, index, arr) => { + if (index > lastVisibleIndex && index > numberOfAlwaysVisibleItems - 1) { + if (startIndex === null) { + startIndex = index; + } + const labelProp = item?.props?.['data-accessible-name'] ? 'accessibleName' : 'aria-label'; + let labelVal = i18nBundle.getText(X_OF_Y, index + 1 - startIndex, arr.length - startIndex); + if (item?.props?.[labelProp]) { + labelVal += ' ' + item.props[labelProp]; + } + // @ts-expect-error: if props is not defined, it doesn't have an id (is not a ReactElement) + if (item?.props?.id) { + // @ts-expect-error: item is ReactElement + return cloneElement(item, { + id: `${item.props.id}-overflow`, + [labelProp]: labelVal + }); + } + // @ts-expect-error: if type is not defined, it's not a spacer + if (item.type?.displayName === 'ToolbarSeparator') { + return cloneElement(item as ReactElement, { + style: { + height: '0.0625rem', + margin: '0.375rem 0.1875rem', + width: '100%' + }, + 'aria-label': labelVal + }); + } + if (isValidElement(item)) { + return cloneElement(item, { + [labelProp]: labelVal + }); + } + } + return null; + }) + .filter(Boolean); + return ( {overflowButton ? ( @@ -152,6 +193,8 @@ export const OverflowPopover: FC = (props: OverflowPopover onOpen={handleAfterOpen} hideArrow accessibleRole={accessibleRole} + //todo translation + accessibleName={`with ${filteredChildrenArray.length} items`} >
= (props: OverflowPopover role={a11yConfig?.overflowPopover?.contentRole} data-component-name="ToolbarOverflowPopoverContent" > - {children.map((item, index) => { - if (index > lastVisibleIndex && index > numberOfAlwaysVisibleItems - 1) { - // @ts-expect-error: if props is not defined, it doesn't have an id (is not a ReactElement) - if (item?.props?.id) { - // @ts-expect-error: item is ReactElement - return cloneElement(item, { id: `${item.props.id}-overflow` }); - } - // @ts-expect-error: if type is not defined, it's not a spacer - if (item.type?.displayName === 'ToolbarSeparator') { - return cloneElement(item as ReactElement, { - style: { - height: '0.0625rem', - margin: '0.375rem 0.1875rem', - width: '100%' - } - }); - } - return item; - } - return null; - })} + {filteredChildrenArray}
, portalContainer ?? document.body diff --git a/packages/compat/src/components/Toolbar/Toolbar.stories.tsx b/packages/compat/src/components/Toolbar/Toolbar.stories.tsx index 850883976bf..4baefb1b213 100644 --- a/packages/compat/src/components/Toolbar/Toolbar.stories.tsx +++ b/packages/compat/src/components/Toolbar/Toolbar.stories.tsx @@ -51,11 +51,15 @@ export const Default: Story = { return ( Toolbar - - - - - + + + + + ); } @@ -67,9 +71,11 @@ export const RightAlignedItems: Story = { return ( - - - + + + ); } @@ -82,11 +88,13 @@ export const EvenlyAlignedItems: Story = { Left - + Right - - + + ); } @@ -97,16 +105,30 @@ export const WithSeparator: Story = { render(args) { return ( - - - + + + - - + + - + - + ); } @@ -125,9 +147,18 @@ export const PopoverInOverflowPopover: Story = { <> Toolbar - - - + + @@ -158,15 +189,25 @@ export const WithOverflowButton: Story = { Toolbar - - - + + + + + Edit + + Favorite @@ -180,32 +221,54 @@ export const OverflowBtns: Story = { render(args) { return ( - - + OverflowToolbarButton (only visible in popover) - + Default ToggleButton OverflowToolbarToggleButton (only visible in popover) - - + OverflowToolbarButton (only visible in popover) - + Default ToggleButton ((props, ref) => { const overflowBtnRef = useRef(null); const [minWidth, setMinWidth] = useState('0'); - const i18nBundle = useI18nBundle('@ui5/webcomponents-react'); - const showMoreText = i18nBundle.getText(SHOW_MORE); - const toolbarClasses = clsx( classNames.outerContainer, toolbarStyle === ToolbarStyle.Clear && classNames.clear, @@ -412,7 +402,6 @@ const Toolbar = forwardRef((props, ref) => { portalContainer={portalContainer} overflowContentRef={overflowContentRef} numberOfAlwaysVisibleItems={numberOfAlwaysVisibleItems} - showMoreText={showMoreText} overflowButton={overflowButton} setIsMounted={setIsPopoverMounted} a11yConfig={a11yConfig} From a9081b05d04503732321a7059ae643a17f9d6498 Mon Sep 17 00:00:00 2001 From: Lukas Harbarth Date: Tue, 22 Oct 2024 16:13:12 +0200 Subject: [PATCH 2/6] add translations --- packages/compat/src/components/Toolbar/OverflowPopover.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/compat/src/components/Toolbar/OverflowPopover.tsx b/packages/compat/src/components/Toolbar/OverflowPopover.tsx index 02551612525..6f1f6cd4e5d 100644 --- a/packages/compat/src/components/Toolbar/OverflowPopover.tsx +++ b/packages/compat/src/components/Toolbar/OverflowPopover.tsx @@ -9,7 +9,7 @@ import type { ToggleButtonPropTypes } from '@ui5/webcomponents-react'; import { Popover, ToggleButton } from '@ui5/webcomponents-react'; -import { SHOW_MORE, X_OF_Y } from '@ui5/webcomponents-react/dist/i18n/i18n-defaults.js'; +import { WITH_X_ITEMS, SHOW_MORE, X_OF_Y } from '@ui5/webcomponents-react/dist/i18n/i18n-defaults.js'; import { stopPropagation } from '@ui5/webcomponents-react/dist/internal/stopPropagation.js'; import { getUi5TagWithSuffix } from '@ui5/webcomponents-react/dist/internal/utils.js'; import { Device, useI18nBundle, useSyncRef } from '@ui5/webcomponents-react-base'; @@ -194,7 +194,7 @@ export const OverflowPopover: FC = (props: OverflowPopover hideArrow accessibleRole={accessibleRole} //todo translation - accessibleName={`with ${filteredChildrenArray.length} items`} + accessibleName={i18nBundle.getText(WITH_X_ITEMS, filteredChildrenArray.length)} >
Date: Tue, 22 Oct 2024 16:21:44 +0200 Subject: [PATCH 3/6] cleanup + story --- .../src/components/Toolbar/OverflowPopover.tsx | 1 - .../compat/src/components/Toolbar/Toolbar.mdx | 18 ++++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/packages/compat/src/components/Toolbar/OverflowPopover.tsx b/packages/compat/src/components/Toolbar/OverflowPopover.tsx index 6f1f6cd4e5d..6e80d0bc01d 100644 --- a/packages/compat/src/components/Toolbar/OverflowPopover.tsx +++ b/packages/compat/src/components/Toolbar/OverflowPopover.tsx @@ -193,7 +193,6 @@ export const OverflowPopover: FC = (props: OverflowPopover onOpen={handleAfterOpen} hideArrow accessibleRole={accessibleRole} - //todo translation accessibleName={i18nBundle.getText(WITH_X_ITEMS, filteredChildrenArray.length)} >
+## Announce number of items in overflow popover + +To set the `aria-label` correctly it's necessary to add the `data-accessible-name` data-attribute for each web component that relies on `accessibleName` instead of `aria-label`. + +E.g.: + +```jsx + + Toolbar + + + + + +``` + ## Prevent event bubbling of Toolbar items Per default, if the `active` prop is "true" and an actionable element like a button is clicked, the `onClick` event of the `Toolbar` is also fired. From 7f018cffc5d2871f28472cbd36766afe55739155 Mon Sep 17 00:00:00 2001 From: Lukas Harbarth Date: Tue, 22 Oct 2024 16:34:11 +0200 Subject: [PATCH 4/6] fix lint + ts errors --- .../src/components/Toolbar/OverflowPopover.tsx | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/packages/compat/src/components/Toolbar/OverflowPopover.tsx b/packages/compat/src/components/Toolbar/OverflowPopover.tsx index 6e80d0bc01d..e106f44ab0a 100644 --- a/packages/compat/src/components/Toolbar/OverflowPopover.tsx +++ b/packages/compat/src/components/Toolbar/OverflowPopover.tsx @@ -14,7 +14,7 @@ import { stopPropagation } from '@ui5/webcomponents-react/dist/internal/stopProp import { getUi5TagWithSuffix } from '@ui5/webcomponents-react/dist/internal/utils.js'; import { Device, useI18nBundle, useSyncRef } from '@ui5/webcomponents-react-base'; import { clsx } from 'clsx'; -import type { Dispatch, FC, ReactElement, ReactNode, Ref, SetStateAction } from 'react'; +import type { Dispatch, FC, HTMLAttributes, ReactElement, ReactNode, Ref, SetStateAction } from 'react'; import { isValidElement, cloneElement, useEffect, useRef, useState } from 'react'; import { createPortal } from 'react-dom'; import { getOverflowPopoverContext } from '../../internal/OverflowPopoverContext.js'; @@ -126,7 +126,7 @@ export const OverflowPopover: FC = (props: OverflowPopover let startIndex = null; const filteredChildrenArray = children .map((item, index, arr) => { - if (index > lastVisibleIndex && index > numberOfAlwaysVisibleItems - 1) { + if (index > lastVisibleIndex && index > numberOfAlwaysVisibleItems - 1 && isValidElement(item)) { if (startIndex === null) { startIndex = index; } @@ -135,10 +135,8 @@ export const OverflowPopover: FC = (props: OverflowPopover if (item?.props?.[labelProp]) { labelVal += ' ' + item.props[labelProp]; } - // @ts-expect-error: if props is not defined, it doesn't have an id (is not a ReactElement) if (item?.props?.id) { - // @ts-expect-error: item is ReactElement - return cloneElement(item, { + return cloneElement>(item, { id: `${item.props.id}-overflow`, [labelProp]: labelVal }); @@ -154,11 +152,6 @@ export const OverflowPopover: FC = (props: OverflowPopover 'aria-label': labelVal }); } - if (isValidElement(item)) { - return cloneElement(item, { - [labelProp]: labelVal - }); - } } return null; }) From a674bf3439050aff9674274854422a0585d49da2 Mon Sep 17 00:00:00 2001 From: Lukas Harbarth Date: Tue, 22 Oct 2024 17:15:16 +0200 Subject: [PATCH 5/6] fix toolbar and tests --- packages/compat/src/components/Toolbar/OverflowPopover.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/compat/src/components/Toolbar/OverflowPopover.tsx b/packages/compat/src/components/Toolbar/OverflowPopover.tsx index e106f44ab0a..838e88efb85 100644 --- a/packages/compat/src/components/Toolbar/OverflowPopover.tsx +++ b/packages/compat/src/components/Toolbar/OverflowPopover.tsx @@ -152,6 +152,9 @@ export const OverflowPopover: FC = (props: OverflowPopover 'aria-label': labelVal }); } + return cloneElement>(item, { + [labelProp]: labelVal + }); } return null; }) From 426010bd72ecb61d58c62c984c977659067b7258 Mon Sep 17 00:00:00 2001 From: Lukas Harbarth Date: Wed, 23 Oct 2024 08:10:40 +0200 Subject: [PATCH 6/6] suppress React 19 ts errors --- packages/compat/src/components/Toolbar/OverflowPopover.tsx | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/compat/src/components/Toolbar/OverflowPopover.tsx b/packages/compat/src/components/Toolbar/OverflowPopover.tsx index 838e88efb85..aa497661cf2 100644 --- a/packages/compat/src/components/Toolbar/OverflowPopover.tsx +++ b/packages/compat/src/components/Toolbar/OverflowPopover.tsx @@ -135,8 +135,13 @@ export const OverflowPopover: FC = (props: OverflowPopover if (item?.props?.[labelProp]) { labelVal += ' ' + item.props[labelProp]; } + + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore: React 19 if (item?.props?.id) { return cloneElement>(item, { + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore: React 19 id: `${item.props.id}-overflow`, [labelProp]: labelVal }); @@ -144,6 +149,8 @@ export const OverflowPopover: FC = (props: OverflowPopover // @ts-expect-error: if type is not defined, it's not a spacer if (item.type?.displayName === 'ToolbarSeparator') { return cloneElement(item as ReactElement, { + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore: React 19 style: { height: '0.0625rem', margin: '0.375rem 0.1875rem',