-
Notifications
You must be signed in to change notification settings - Fork 887
Fix EuiAccordion zero height under React Strict Mode #9811
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
f1b6b68
ce8e334
f57192d
30cda29
db1b183
870563a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| **Bug fixes** | ||
|
|
||
| - Fixed `EuiAccordion` rendering with zero height under React Strict Mode (including when mounted inside `EuiFlyout` with `initialIsOpen`) |
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. blocking: For testing the result it's fine but before I approve, we have to remove these stories. We don't need StrictMode specific ones. |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -93,10 +93,19 @@ export const EuiAccordionChildren: FunctionComponent< | |||||||||||||||||||||||||||||||||||||||
| ({ height }: { height: number }) => setContentHeight(Math.round(height)), | ||||||||||||||||||||||||||||||||||||||||
| [] | ||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||
| const heightInlineStyle = useMemo( | ||||||||||||||||||||||||||||||||||||||||
| () => ({ blockSize: isOpen ? contentHeight : 0 }), | ||||||||||||||||||||||||||||||||||||||||
| [isOpen, contentHeight] | ||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||
| const heightInlineStyle = useMemo(() => { | ||||||||||||||||||||||||||||||||||||||||
| if (!isOpen) { | ||||||||||||||||||||||||||||||||||||||||
| return { blockSize: 0 }; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| // Avoid overriding CSS `height: auto` with `0` before the first resize | ||||||||||||||||||||||||||||||||||||||||
| // measurement (e.g. `initialIsOpen` under React Strict Mode). | ||||||||||||||||||||||||||||||||||||||||
| if (contentHeight === 0) { | ||||||||||||||||||||||||||||||||||||||||
| return undefined; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| return { blockSize: contentHeight }; | ||||||||||||||||||||||||||||||||||||||||
| }, [isOpen, contentHeight]); | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+96
to
+108
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: This is very opinionated but we could tighten this like so:
Suggested change
instead of having 13 lines we have 6 and the code is equally meaningful. |
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||||||||||||||
| <div | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. blocking: We can add a small targeted unit test in the existing |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
| * in compliance with, at your election, the Elastic License 2.0 or the Server | ||
| * Side Public License, v 1. | ||
| */ | ||
|
|
||
| import React, { StrictMode } from 'react'; | ||
| import { render, screen } from '../../test/rtl'; | ||
| import { createResizeObserverMock } from '../../test/internal'; | ||
| import { EuiAccordion } from './accordion'; | ||
| import { EuiFlyout, EuiFlyoutBody } from '../flyout'; | ||
|
|
||
| jest.mock('../portal', () => ({ | ||
| EuiPortal: ({ children }: { children: React.ReactNode }) => children, | ||
| })); | ||
|
|
||
| const resizeObserverMockRef: { | ||
| current: ReturnType<typeof createResizeObserverMock> | null; | ||
| } = { current: null }; | ||
|
|
||
| jest.mock('../observer/resize_observer/resize_observer.tsx', () => { | ||
| const { createResizeObserverMock } = jest.requireActual< | ||
| typeof import('../../test/internal') | ||
| >('../../test/internal'); | ||
| resizeObserverMockRef.current = createResizeObserverMock(); | ||
| global.ResizeObserver = resizeObserverMockRef.current.ResizeObserver; | ||
|
|
||
| return jest.requireActual('../observer/resize_observer/resize_observer.tsx'); | ||
| }); | ||
|
Comment on lines
+23
to
+31
|
||
|
|
||
| const createMockEntry = ( | ||
| element: Element, | ||
| { width = 500, height = 100 } = {} | ||
| ): ResizeObserverEntry => ({ | ||
| target: element, | ||
| contentRect: new DOMRect(0, 0, width, height), | ||
| borderBoxSize: [{ inlineSize: width, blockSize: height }], | ||
| contentBoxSize: [], | ||
| devicePixelContentBoxSize: [], | ||
| }); | ||
|
|
||
| const ISSUE_9029_CONTENT = 'Accordion content inside flyout (issue #9029)'; | ||
|
|
||
| const renderAccordionInFlyout = () => | ||
| render( | ||
| <StrictMode> | ||
| <EuiFlyout onClose={() => {}} aria-label="Flyout"> | ||
| <EuiFlyoutBody> | ||
| <EuiAccordion | ||
| id="accordion-in-flyout-9029" | ||
| initialIsOpen | ||
| buttonContent="Accordion in flyout" | ||
| > | ||
| <div>{ISSUE_9029_CONTENT}</div> | ||
| </EuiAccordion> | ||
| </EuiFlyoutBody> | ||
| </EuiFlyout> | ||
| </StrictMode> | ||
| ); | ||
|
|
||
| describe('EuiAccordion Strict Mode regressions', () => { | ||
| beforeEach(() => { | ||
| resizeObserverMockRef.current?.ResizeObserver.mockClear(); | ||
| }); | ||
|
|
||
| /** | ||
| * https://github.com/elastic/eui/issues/9029 | ||
| */ | ||
| it('renders open accordion content inside EuiFlyout under Strict Mode', () => { | ||
| const resizeObserverMock = resizeObserverMockRef.current!; | ||
| renderAccordionInFlyout(); | ||
|
|
||
| const content = screen.getByText(ISSUE_9029_CONTENT); | ||
| const childWrapper = content.closest( | ||
| '.euiAccordion__childWrapper' | ||
| ) as HTMLElement; | ||
|
|
||
| expect(childWrapper).toBeInTheDocument(); | ||
| expect(childWrapper).not.toHaveStyle({ blockSize: '0px' }); | ||
| expect(content).toBeVisible(); | ||
|
|
||
| // Simulate resize notifications after Strict Mode observer reconnect | ||
| const observers = resizeObserverMock.ResizeObserver.mock.results.map( | ||
| (result) => result.value | ||
| ); | ||
| const observedElement = childWrapper.querySelector( | ||
| '.euiAccordion__children' | ||
| ) as HTMLElement; | ||
|
|
||
| observers.forEach((observer) => { | ||
| resizeObserverMock.triggerCallback( | ||
| [createMockEntry(observedElement)], | ||
| observer | ||
| ); | ||
| }); | ||
|
|
||
| expect(childWrapper).not.toHaveStyle({ blockSize: '0px' }); | ||
| expect(content).toBeVisible(); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blocking:
The filename is supposed to be
9811.md, from the PR number.