From 86fe018577b24adf3b7ff7c7631aed36b7caa466 Mon Sep 17 00:00:00 2001 From: Tasso Evangelista Date: Wed, 23 Oct 2024 20:10:01 -0300 Subject: [PATCH] feat(fuselage)!: Lighter `Accordion` and `AccordionItem` components (#1470) --- .changeset/soft-islands-decide.md | 13 + packages/fuselage/jest-setup.ts | 6 + .../components/Accordion/Accordion.spec.tsx | 26 +- .../Accordion/Accordion.stories.tsx | 54 ++- .../Accordion/Accordion.styles.scss | 9 - .../src/components/Accordion/Accordion.tsx | 27 +- .../components/Accordion/AccordionItem.tsx | 114 +++--- .../__snapshots__/Accordion.spec.tsx.snap | 375 ++++++++++++++++++ .../src/components/Accordion/index.ts | 7 +- .../src/components/Box/StylingBox.tsx | 34 ++ packages/fuselage/src/components/Box/index.ts | 2 + .../fuselage/src/components/Box/index.tsx | 1 - .../fuselage/src/helpers/composeClassNames.ts | 95 +++++ .../fuselage/src/helpers/exhaustiveCheck.ts | 30 ++ 14 files changed, 682 insertions(+), 111 deletions(-) create mode 100644 .changeset/soft-islands-decide.md create mode 100644 packages/fuselage/src/components/Accordion/__snapshots__/Accordion.spec.tsx.snap create mode 100644 packages/fuselage/src/components/Box/StylingBox.tsx create mode 100644 packages/fuselage/src/components/Box/index.ts delete mode 100644 packages/fuselage/src/components/Box/index.tsx create mode 100644 packages/fuselage/src/helpers/exhaustiveCheck.ts diff --git a/.changeset/soft-islands-decide.md b/.changeset/soft-islands-decide.md new file mode 100644 index 0000000000..22a87e4dcb --- /dev/null +++ b/.changeset/soft-islands-decide.md @@ -0,0 +1,13 @@ +--- +"@rocket.chat/fuselage": minor +--- + +Simplifies `Accordion` and `AccordionItem` + +It removes an obsolete and not accessible toggle switch in `AccordionItem` and eases the internal usage of `Box` to +improve rendering performance. + +Additionally, it adds a new `StylingBox` component that can be used as a wrapper for components that accept styling +props but don't need the weight of the `Box` component prop handling internally. + +Also, it adds a new `cx` and `cxx` helpers to compose class names. diff --git a/packages/fuselage/jest-setup.ts b/packages/fuselage/jest-setup.ts index cf23e03cf2..eb28fb96a5 100644 --- a/packages/fuselage/jest-setup.ts +++ b/packages/fuselage/jest-setup.ts @@ -48,3 +48,9 @@ window.ResizeObserver = jest.fn().mockImplementation(() => ({ unobserve: jest.fn(), disconnect: jest.fn(), })); + +let uniqueIdCounter = 0; +jest.mock('@rocket.chat/fuselage-hooks', () => ({ + ...jest.requireActual('@rocket.chat/fuselage-hooks'), + useUniqueId: () => `unique-id-${uniqueIdCounter++}`, +})); diff --git a/packages/fuselage/src/components/Accordion/Accordion.spec.tsx b/packages/fuselage/src/components/Accordion/Accordion.spec.tsx index dd89c59947..6906ff8101 100644 --- a/packages/fuselage/src/components/Accordion/Accordion.spec.tsx +++ b/packages/fuselage/src/components/Accordion/Accordion.spec.tsx @@ -4,17 +4,25 @@ import { axe } from 'jest-axe'; import { render } from '../../testing'; import * as stories from './Accordion.stories'; -const { Default } = composeStories(stories); +const testCases = Object.values(composeStories(stories)).map((Story) => [ + Story.storyName || 'Story', + Story, +]); -describe('[Accordion Component]', () => { - it('renders without crashing', () => { - render(); - }); +test.each(testCases)( + `renders %s without crashing`, + async (_storyname, Story) => { + const tree = render(); + expect(tree.baseElement).toMatchSnapshot(); + } +); - it('should have no a11y violations', async () => { - const { container } = render(); +test.each(testCases)( + '%s should have no a11y violations', + async (_storyname, Story) => { + const { container } = render(); const results = await axe(container); expect(results).toHaveNoViolations(); - }); -}); + } +); diff --git a/packages/fuselage/src/components/Accordion/Accordion.stories.tsx b/packages/fuselage/src/components/Accordion/Accordion.stories.tsx index a9593ba888..209f9fa92e 100644 --- a/packages/fuselage/src/components/Accordion/Accordion.stories.tsx +++ b/packages/fuselage/src/components/Accordion/Accordion.stories.tsx @@ -2,36 +2,66 @@ import type { StoryFn, Meta } from '@storybook/react'; import type { ComponentType } from 'react'; import Box from '../Box'; -import { Accordion } from './Accordion'; -import { AccordionItem } from './AccordionItem'; +import Accordion from './Accordion'; +import AccordionItem from './AccordionItem'; export default { title: 'Containers/Accordion', component: Accordion, subcomponents: { - 'Accordion.Item': Accordion.Item as ComponentType, - 'AccordionItem': AccordionItem as ComponentType, + AccordionItem: AccordionItem as ComponentType, }, } satisfies Meta; -const Template: StoryFn = () => ( +export const Default: StoryFn = () => ( - + Content #1 - - + + Content #2 - - + + Content #3 - + ); -export const Default: StoryFn = Template.bind({}); +const ItemTemplate: StoryFn = ({ + title = 'Item #2', + ...args +}) => ( + + + + Content #1 + + + + + Content #2 + + + + + Content #3 + + + +); + +export const ExpandedItemByDefault = ItemTemplate.bind({}); +ExpandedItemByDefault.args = { + defaultExpanded: true, +}; + +export const DisabledItem = ItemTemplate.bind({}); +DisabledItem.args = { + disabled: true, +}; diff --git a/packages/fuselage/src/components/Accordion/Accordion.styles.scss b/packages/fuselage/src/components/Accordion/Accordion.styles.scss index 8e591e47ac..962eedaeff 100644 --- a/packages/fuselage/src/components/Accordion/Accordion.styles.scss +++ b/packages/fuselage/src/components/Accordion/Accordion.styles.scss @@ -6,7 +6,6 @@ display: flex; flex-flow: column nowrap; border-block-end-color: colors.stroke(extra-light); - border-block-end-width: lengths.border-width(default); } @@ -64,14 +63,6 @@ @include typography.use-font-scale(h4); } -.rcx-accordion-item__toggle-switch { - display: flex; - align-items: center; - flex: 0 0 auto; - - margin: lengths.margin(none) lengths.margin(24); -} - .rcx-accordion-item__panel { visibility: hidden; diff --git a/packages/fuselage/src/components/Accordion/Accordion.tsx b/packages/fuselage/src/components/Accordion/Accordion.tsx index 57d0a552eb..8de50c0ab8 100644 --- a/packages/fuselage/src/components/Accordion/Accordion.tsx +++ b/packages/fuselage/src/components/Accordion/Accordion.tsx @@ -1,21 +1,22 @@ -import type { ComponentProps, ReactElement, ReactNode } from 'react'; +import type { ReactNode } from 'react'; -import Box from '../Box'; -import { AccordionItem } from './AccordionItem'; +import { cx, cxx } from '../../helpers/composeClassNames'; +import { StylingBox } from '../Box'; +import { StylingProps } from '../Box/stylingProps'; -type AccordionProps = ComponentProps & { - animated?: boolean; +export type AccordionProps = { children: ReactNode; -}; +} & Partial; /** * An `Accordion` allows users to toggle the display of sections of content. */ -export function Accordion(props: AccordionProps): ReactElement { - return ; -} +const Accordion = ({ children, ...props }: AccordionProps) => ( + +
+ {children} +
+ +); -/** - * @deprecated use named import instead - */ -Accordion.Item = AccordionItem; +export default Accordion; diff --git a/packages/fuselage/src/components/Accordion/AccordionItem.tsx b/packages/fuselage/src/components/Accordion/AccordionItem.tsx index 94081394b8..1af15408e3 100644 --- a/packages/fuselage/src/components/Accordion/AccordionItem.tsx +++ b/packages/fuselage/src/components/Accordion/AccordionItem.tsx @@ -1,11 +1,11 @@ import { useToggle, useUniqueId } from '@rocket.chat/fuselage-hooks'; -import type { FormEvent, KeyboardEvent, MouseEvent, ReactNode } from 'react'; +import type { KeyboardEvent, MouseEvent, ReactNode } from 'react'; -import Box from '../Box'; +import { cx, cxx } from '../../helpers/composeClassNames'; +import { StylingBox } from '../Box'; import { Chevron } from '../Chevron'; -import { ToggleSwitch } from '../ToggleSwitch'; -type AccordionItemProps = { +export type AccordionItemProps = { children?: ReactNode; className?: string; defaultExpanded?: boolean; @@ -14,33 +14,20 @@ type AccordionItemProps = { tabIndex?: number; title: ReactNode; noncollapsible?: boolean; - onToggle?: (e: MouseEvent | KeyboardEvent) => void; - onToggleEnabled?: (e: FormEvent) => void; }; -export const AccordionItem = function Item({ +const AccordionItem = ({ children, - className, defaultExpanded, - disabled, + disabled = false, expanded: propExpanded, tabIndex = 0, title, noncollapsible = !title, - onToggle, - onToggleEnabled, ...props -}: AccordionItemProps) { +}: AccordionItemProps) => { const [stateExpanded, toggleStateExpanded] = useToggle(defaultExpanded); const expanded = propExpanded || stateExpanded; - const toggleExpanded = (event: MouseEvent | KeyboardEvent) => { - if (onToggle) { - onToggle.call(event.currentTarget, event); - return; - } - - toggleStateExpanded(); - }; const panelExpanded = noncollapsible || expanded; @@ -52,7 +39,7 @@ export const AccordionItem = function Item({ return; } e.currentTarget?.blur(); - toggleExpanded(e); + toggleStateExpanded(); }; const handleKeyDown = (event: KeyboardEvent) => { @@ -60,19 +47,17 @@ export const AccordionItem = function Item({ return; } - if ([13, 32].includes(event.keyCode)) { - event.preventDefault(); + if (![' ', 'Enter'].includes(event.key)) { + return; + } - if (event.repeat) { - return; - } + event.preventDefault(); - toggleExpanded(event); + if (event.repeat) { + return; } - }; - const handleToggleClick = (event: MouseEvent) => { - event.stopPropagation(); + toggleStateExpanded(); }; const collapsibleProps = { @@ -92,42 +77,41 @@ export const AccordionItem = function Item({ const barProps = noncollapsible ? nonCollapsibleProps : collapsibleProps; return ( - - {title && ( - - - {title} - - {!noncollapsible && ( - <> - {(disabled || onToggleEnabled) && ( - - - + +
+ {title && ( +
+

- + id={titleId} + > + {title} +

+ {!noncollapsible && } +
+ )} +
- )} - - {children} - - + id={panelId} + > + {children} +
+
+ ); }; + +export default AccordionItem; diff --git a/packages/fuselage/src/components/Accordion/__snapshots__/Accordion.spec.tsx.snap b/packages/fuselage/src/components/Accordion/__snapshots__/Accordion.spec.tsx.snap new file mode 100644 index 0000000000..47f51c1482 --- /dev/null +++ b/packages/fuselage/src/components/Accordion/__snapshots__/Accordion.spec.tsx.snap @@ -0,0 +1,375 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`renders Default without crashing 1`] = ` + +
+
+
+ +
+
+ Content #1 +
+
+
+
+ +
+
+ Content #2 +
+
+
+
+ +
+
+ Content #3 +
+
+
+
+
+ +`; + +exports[`renders DisabledItem without crashing 1`] = ` + +
+
+
+ +
+
+ Content #1 +
+
+
+
+ +
+
+ Content #2 +
+
+
+
+ +
+
+ Content #3 +
+
+
+
+
+ +`; + +exports[`renders ExpandedItemByDefault without crashing 1`] = ` + +
+
+
+ +
+
+ Content #1 +
+
+
+
+
+

+ Item #2 +

+ + + +
+
+
+ Content #2 +
+
+
+
+ +
+
+ Content #3 +
+
+
+
+
+ +`; diff --git a/packages/fuselage/src/components/Accordion/index.ts b/packages/fuselage/src/components/Accordion/index.ts index cadd5bc406..fb3ec96a24 100644 --- a/packages/fuselage/src/components/Accordion/index.ts +++ b/packages/fuselage/src/components/Accordion/index.ts @@ -1,2 +1,5 @@ -export * from './Accordion'; -export * from './AccordionItem'; +export { default as Accordion, type AccordionProps } from './Accordion'; +export { + default as AccordionItem, + type AccordionItemProps, +} from './AccordionItem'; diff --git a/packages/fuselage/src/components/Box/StylingBox.tsx b/packages/fuselage/src/components/Box/StylingBox.tsx new file mode 100644 index 0000000000..b18fd4bb4b --- /dev/null +++ b/packages/fuselage/src/components/Box/StylingBox.tsx @@ -0,0 +1,34 @@ +import type { cssFn } from '@rocket.chat/css-in-js'; +import { cloneElement } from 'react'; +import type { ReactElement } from 'react'; + +import { useArrayLikeClassNameProp } from '../../hooks/useArrayLikeClassNameProp'; +import type { Falsy } from '../../types/Falsy'; +import type { StylingProps } from './stylingProps'; +import { useStylingProps } from './useStylingProps'; + +export type StylingBoxProps = { + children: ReactElement<{ className?: string }>; + className?: string | cssFn | (string | cssFn | Falsy)[]; +} & Partial; + +/** + * Merges its `StylingProps` props into a `className` prop passed to a child element. + * + * This is intended to be used as a wrapper for components that accept styling props but don't need the weight of the + * `Box` component prop handling internally. + */ +export const StylingBox = ({ children, ...props }: StylingBoxProps) => { + const propsWithStringClassName = useArrayLikeClassNameProp(props); + propsWithStringClassName.className = [ + children.props.className, + propsWithStringClassName.className, + ] + .filter(Boolean) + .join(' '); + const propsWithoutStylingProps = useStylingProps(propsWithStringClassName); + + return cloneElement(children, propsWithoutStylingProps); +}; + +export default StylingBox; diff --git a/packages/fuselage/src/components/Box/index.ts b/packages/fuselage/src/components/Box/index.ts new file mode 100644 index 0000000000..b2f4651178 --- /dev/null +++ b/packages/fuselage/src/components/Box/index.ts @@ -0,0 +1,2 @@ +export { default } from './Box'; +export { default as StylingBox, type StylingBoxProps } from './StylingBox'; diff --git a/packages/fuselage/src/components/Box/index.tsx b/packages/fuselage/src/components/Box/index.tsx deleted file mode 100644 index a3e93ac535..0000000000 --- a/packages/fuselage/src/components/Box/index.tsx +++ /dev/null @@ -1 +0,0 @@ -export { default } from './Box'; diff --git a/packages/fuselage/src/helpers/composeClassNames.ts b/packages/fuselage/src/helpers/composeClassNames.ts index 704b1fccb4..8ac964b71c 100644 --- a/packages/fuselage/src/helpers/composeClassNames.ts +++ b/packages/fuselage/src/helpers/composeClassNames.ts @@ -1,3 +1,6 @@ +import { Falsy } from '../types/Falsy'; +import { exhaustiveCheck } from './exhaustiveCheck'; + const withPrefix = (prefix?: string) => (modifier: string) => prefix ? `${prefix}--${modifier}` : modifier; @@ -45,3 +48,95 @@ export const composeClassNames = return [prefix, classNames].filter(Boolean).join(' '); }; + +type CxArg = + | Falsy + | string + | { [className: string]: boolean | string | number }; + +/** + * Composes class names into a single string based on flags. + * + * Any falsy value passed as an argument will be ignored. + * + * If a string is passed, it will be used as a class name. + * + * If a dictionary object is passed, its keys will be used as class names and its values will be used as modifiers -- + * unless the value is a boolean, in which case the key will be used as a class name if the value is `true`. + * + * @example + * ```ts + * cx('a', false, 'b', null, { c: true, d: false, e: 'f', f: 1 }); // 'a b c e-f f-1' + * ``` + * + * @param args a sequence of falsy values, strings and dictionary objects containing the class names + * @returns a space-separated string of class names + */ +export const cx = (...args: CxArg[]): string => + args + .flatMap((arg) => { + if (!arg) { + return []; + } + + if (typeof arg === 'string') { + return arg; + } + + if (typeof arg === 'object') { + return Object.entries(arg).flatMap(([className, value]) => { + if (typeof value === 'boolean') { + return value ? className : []; + } + + return `${className}-${value}`; + }); + } + + return exhaustiveCheck(arg); + }) + .join(' '); + +/** + * Composes class name modifiers under a class name into a single class names' string based on flags. + * + * This function returns a function similar to `cx`, with the difference it handles the arguments as class name + * modifiers (based on BEM CSS conventions). However, it's recommended to use `cxx` as a curried function. + * + * @example + * ```ts + * cxx('z')('a', false, 'b', null, { c: true, d: false, e: 'f', f: 1 }); // 'z z--a z--b z--c z--e-f z--f-1' + * ``` + * @param className the class name + * @param args a sequence of falsy values, strings and dictionary objects containing the modifiers + * @returns a space-separated string of class names + */ +export const cxx = + (className: string) => + (...args: CxArg[]): string => { + const classNames = args.flatMap((arg) => { + if (!arg) { + return []; + } + + if (typeof arg === 'string') { + return `${className}--${arg}`; + } + + if (typeof arg === 'object') { + return Object.entries(arg).flatMap(([modifier, value]) => { + if (typeof value === 'boolean') { + return value ? `${className}--${modifier}` : []; + } + + return `${className}--${modifier}-${value}`; + }); + } + + return exhaustiveCheck(arg); + }); + + classNames.unshift(className); + + return classNames.join(' '); + }; diff --git a/packages/fuselage/src/helpers/exhaustiveCheck.ts b/packages/fuselage/src/helpers/exhaustiveCheck.ts new file mode 100644 index 0000000000..0f32cd27f5 --- /dev/null +++ b/packages/fuselage/src/helpers/exhaustiveCheck.ts @@ -0,0 +1,30 @@ +/** + * Utility function to check exhaustiveness of a switch statement or if-else statements. + * + * In TypeScript, variables going through a sequence of conditionals, like cases in switch statements or if-else + * statements, the variable type is incrementally narrowed as the conditionals are evaluated until it reaches the + * `never` type, meaning all possible cases have been covered. This function is used to check, at compilation time, that + * all possible cases have been covered (i.e. if the conditional checks were exhaustive) and should only be called in + * unreachable blocks of code. + * + * @example + * ```ts + * declare const value: 'foo' | 'bar'; + * switch (value) { + * case 'foo': + * // ... + * break; + * case 'bar': + * // ... + * break; + * default: // should be unreachable + * exhaustiveCheck(value); // `value` type should be `never`, otherwise the compilation fails + * } + * ``` + * + * @param _ the value which type should be `never` + * @throws {Error} will always throw an error if it's reached + */ +export const exhaustiveCheck = (_: never): never => { + throw new Error('Exhaustive check failed'); +};