Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[EuiAccordion] Split up rendering into sub-components & other cleanups #7161

Merged
merged 12 commits into from
Sep 8, 2023
Merged

[EuiAccordion] Split up rendering into sub-components & other cleanups #7161

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a73bede
[REFACTOR] Split up accordion trigger into button & arrow sub-components
cee-chen Sep 2, 2023
5ca4269
[rename] Update downstream references to accordion arrow className
cee-chen Sep 4, 2023
255b5fb
[misc cleanup] EuiAccordion - reorder imports & props
cee-chen Sep 2, 2023
84d8479
[REFACTOR] Split up accordion children into sub-components
cee-chen Sep 2, 2023
f7773f4
[refactor] Switch to styles obj instead of modifying a DOM element di…
cee-chen Sep 4, 2023
87f4faa
[refactor] Remove manual tabbable controls and use `inert` attribute …
cee-chen Sep 4, 2023
8567a31
[opinionated refactor] Clean up child wrapper styles
cee-chen Sep 4, 2023
a72aa94
[regression] Fix children wrapper `.focus()` to only occur after mount
cee-chen Sep 4, 2023
3e82e7e
More downstream snapshot updates
cee-chen Sep 4, 2023
e195f62
changelog + minor stylelint cleanup
cee-chen Sep 6, 2023
dbcab02
[PR feedback] Reduce CSS overrides on arrow styles
cee-chen Sep 8, 2023
2d32730
:facepalm: Fix incorrect CSS
cee-chen Sep 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions src/components/accordion/__snapshots__/accordion.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ exports[`EuiAccordion behavior closes when clicked twice 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper"
id="25"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -93,6 +94,7 @@ exports[`EuiAccordion behavior does not open when isDisabled 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper"
id="23"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -145,6 +147,7 @@ exports[`EuiAccordion behavior opens when clicked once 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isOpen"
id="22"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -197,6 +200,7 @@ exports[`EuiAccordion behavior opens when div is clicked if element is a div 1`]
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isOpen"
id="24"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -304,6 +308,7 @@ exports[`EuiAccordion is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper"
id="0"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -354,6 +359,7 @@ exports[`EuiAccordion isDisabled is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper"
id="21"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -387,6 +393,7 @@ exports[`EuiAccordion props arrowDisplay none is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper"
id="14"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -435,6 +442,7 @@ exports[`EuiAccordion props arrowDisplay right is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper"
id="13"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -485,6 +493,7 @@ exports[`EuiAccordion props arrowProps is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper"
id="15"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -537,6 +546,7 @@ exports[`EuiAccordion props buttonContent is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper"
id="3"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -585,6 +595,7 @@ exports[`EuiAccordion props buttonContentClassName is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper"
id="2"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -631,6 +642,7 @@ exports[`EuiAccordion props buttonElement is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper"
id="10"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -681,6 +693,7 @@ exports[`EuiAccordion props buttonProps is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper"
id="4"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -730,6 +743,7 @@ exports[`EuiAccordion props buttonProps paddingSize l 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper"
id="7"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -779,6 +793,7 @@ exports[`EuiAccordion props buttonProps paddingSize m 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper"
id="6"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -828,6 +843,7 @@ exports[`EuiAccordion props buttonProps paddingSize s 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper"
id="5"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -874,6 +890,7 @@ exports[`EuiAccordion props element is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper"
id="1"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -929,6 +946,7 @@ exports[`EuiAccordion props extraAction is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper"
id="11"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -977,6 +995,7 @@ exports[`EuiAccordion props forceState closed is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper"
id="16"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -1029,6 +1048,7 @@ exports[`EuiAccordion props forceState open is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isOpen"
id="17"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -1081,6 +1101,7 @@ exports[`EuiAccordion props initialIsOpen is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isOpen"
id="12"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -1142,6 +1163,7 @@ exports[`EuiAccordion props isLoading is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper"
id="19"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -1199,6 +1221,7 @@ exports[`EuiAccordion props isLoadingMessage is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper"
id="20"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down
39 changes: 15 additions & 24 deletions src/components/accordion/accordion_children/accordion_children.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import React, {
useRef,
useCallback,
useEffect,
useMemo,
useState,
} from 'react';
import classNames from 'classnames';
import { tabbable, FocusableElement } from 'tabbable';
Expand Down Expand Up @@ -58,8 +60,6 @@ export const EuiAccordionChildren: FunctionComponent<
paddingSize && paddingSize !== 'none' && styles[paddingSize],
];

const contentRef = useRef<HTMLDivElement | null>(null);

/**
* Wrapper
*/
Expand All @@ -75,18 +75,15 @@ export const EuiAccordionChildren: FunctionComponent<
* Update the accordion wrapper height whenever the accordion opens, and also
* whenever the child content updates (which will change the height)
*/
const setAccordionHeight = useCallback(() => {
requestAnimationFrame(() => {
if (!contentRef.current || !wrapperRef.current) return;

const height = isOpen ? contentRef.current!.clientHeight : 0;
wrapperRef.current.style.blockSize = `${height}px`;
});
}, [isOpen]);

useEffect(() => {
setAccordionHeight();
}, [setAccordionHeight, children]);
const [contentHeight, setContentHeight] = useState(0);
const onResize = useCallback(
({ height }: { height: number }) => setContentHeight(Math.round(height)),
[]
);
const heightInlineStyle = useMemo(
() => ({ blockSize: isOpen ? contentHeight : 0 }),
[isOpen, contentHeight]
);
Comment on lines +76 to +84
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice refactor!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huzzah, great to hear it! In general I'm a huge fan of how function components and hooks makes us write more modern React best practices :)


/**
* Focus the children wrapper on open
Expand All @@ -105,7 +102,7 @@ export const EuiAccordionChildren: FunctionComponent<
// When accordions are closed, tabbable children should not be present in the tab order
if (!isOpen) {
// Re-check for children on every close - content can change dynamically
tabbableChildren.current = tabbable(contentRef.current!);
tabbableChildren.current = tabbable(wrapperRef.current!);

tabbableChildren.current.forEach((element) => {
// If the element has an existing `tabIndex` set, make sure we can restore it
Expand Down Expand Up @@ -142,20 +139,14 @@ export const EuiAccordionChildren: FunctionComponent<
{...rest}
className="euiAccordion__childWrapper"
css={wrapperCssStyles}
style={heightInlineStyle}
ref={wrapperRef}
tabIndex={-1}
role="region"
>
<EuiResizeObserver onResize={setAccordionHeight}>
<EuiResizeObserver onResize={onResize}>
{(resizeRef) => (
<div
ref={(node) => {
resizeRef(node);
contentRef.current = node;
}}
className={classes}
css={cssStyles}
>
<div ref={resizeRef} className={classes} css={cssStyles}>
{isLoading && isLoadingMessage ? (
<EuiAccordionChildrenLoading
isLoadingMessage={isLoadingMessage}
Expand Down
2 changes: 2 additions & 0 deletions ...sible_nav_beta/collapsible_nav_item/__snapshots__/collapsible_nav_accordion.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ exports[`EuiCollapsibleNavAccordion renders as a sub item 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper"
id="generated-id"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -111,6 +112,7 @@ exports[`EuiCollapsibleNavAccordion renders as a top level item 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper"
id="generated-id"
role="region"
style="block-size: 0;"
tabindex="-1"
>
<div
Expand Down