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

[EuiFlyout] Performance cleanup #7462

Merged
merged 9 commits into from
Mar 14, 2024
Merged

[EuiFlyout] Performance cleanup #7462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6b96eee
Fix performance issue in EuiResizableFlyout that would cause classes …
davismcphee Jan 14, 2024
2f7db69
Add unit tests for euiBody--hasFlyout class
davismcphee Jan 14, 2024
ef70c47
Replace flyout useResizeObserver call with ResizeObserver directly to…
davismcphee Mar 4, 2024
0624fc8
Check for ResizeObserver availability in flyout using hasResizeObserver
davismcphee Mar 4, 2024
f85b22f
Added changelog entry
davismcphee Mar 4, 2024
4750158
Merge branch 'main' into optimize-resizable-flyout-reflows
cee-chen Mar 13, 2024
904323b
[PR updates] Return to using `useResizeObserver` + misc cleanup
cee-chen Mar 13, 2024
22d0659
[PR feedback] Simplify tests
cee-chen Mar 13, 2024
313ae2d
Changelog tweak to reflect new changes
cee-chen Mar 13, 2024
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
3 changes: 3 additions & 0 deletions changelogs/upcoming/7462.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
**Bug fixes**

- Fixed `EuiFlyout` to not repeatedly remove/add a body class on resize
2 changes: 1 addition & 1 deletion src/components/flyout/__snapshots__/flyout.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1070,7 +1070,7 @@ Array [

exports[`EuiFlyout renders extra screen reader instructions when fixed EuiHeaders headers exist on the page 1`] = `
<body
class="euiBody--headerIsFixed euiBody--hasFlyout"
class="euiBody--hasFlyout euiBody--headerIsFixed"
>
<div>
<div
Expand Down
29 changes: 29 additions & 0 deletions src/components/flyout/flyout.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,5 +278,34 @@ describe('EuiFlyout', () => {
).toMatchSnapshot();
});
});

describe('body class', () => {
it('adds `.euiBody--hasFlyout` class on mount', () => {
render(<EuiFlyout onClose={() => {}} />);
expect(document.body).toHaveClass('euiBody--hasFlyout');
});

it('removes `.euiBody--hasFlyout` class on unmount', () => {
const { unmount } = render(<EuiFlyout onClose={() => {}} />);
unmount();
expect(document.body).not.toHaveClass('euiBody--hasFlyout');
});

// Regression testing
it('should not remove and re-add `.euiBody--hasFlyout` class on resize', async () => {
const add = jest.spyOn(document.body.classList, 'add');
const remove = jest.spyOn(document.body.classList, 'remove');
const { rerender } = render(
<EuiFlyout onClose={() => {}} size={500} />
);

expect(add).toHaveBeenCalledTimes(1);
expect(add).toHaveBeenLastCalledWith('euiBody--hasFlyout');

rerender(<EuiFlyout onClose={() => {}} size={600} />);
expect(add).toHaveBeenCalledTimes(1);
expect(remove).not.toHaveBeenCalled();
});
});
});
});
35 changes: 16 additions & 19 deletions src/components/flyout/flyout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,36 +214,33 @@ export const EuiFlyout = forwardRef(
null
);
const setRef = useCombinedRefs([setResizeRef, ref]);
// TODO: Allow this hook to be conditional
const dimensions = useResizeObserver(resizeRef);
const { width } = useResizeObserver(isPushed ? resizeRef : null, 'width');

useEffect(() => {
// This class doesn't actually do anything by EUI, but is nice to add for consumers (JIC)
document.body.classList.add('euiBody--hasFlyout');

/**
* Accomodate for the `isPushed` state by adding padding to the body equal to the width of the element
*/
if (isPushed) {
if (side === 'right') {
document.body.style.paddingInlineEnd = `${dimensions.width}px`;
} else if (side === 'left') {
document.body.style.paddingInlineStart = `${dimensions.width}px`;
}
const paddingSide =
side === 'left' ? 'paddingInlineStart' : 'paddingInlineEnd';

document.body.style[paddingSide] = `${width}px`;
return () => {
document.body.style[paddingSide] = '';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed while testing this gets called on each frame as the width changes when resizing, but it didn't seem to have much if any performance impact in Chrome, Firefox, or Safari.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks a million for checking that Davis! 🙇

};
}
}, [isPushed, side, width]);

/**
* This class doesn't actually do anything by EUI, but is nice to add for consumers (JIC)
*/
useEffect(() => {
document.body.classList.add('euiBody--hasFlyout');
return () => {
// Remove the hasFlyout class when the flyout is unmounted
document.body.classList.remove('euiBody--hasFlyout');

if (isPushed) {
if (side === 'right') {
document.body.style.paddingInlineEnd = '';
} else if (side === 'left') {
document.body.style.paddingInlineStart = '';
}
}
};
}, [side, dimensions, isPushed]);
}, []);

/**
* ESC key closes flyout (always?)
Expand Down
Loading