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

[EuiInputPopover] Refactor & clean ups #7241

Merged
merged 6 commits into from
Oct 2, 2023
Merged

[EuiInputPopover] Refactor & clean ups #7241

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8253ede
[nit] Reorganize import order
cee-chen Oct 1, 2023
9ba6fa9
Refactor `EuiResizeObserver` to `useResizeObserver`
cee-chen Oct 1, 2023
3283c1d
Clean up and memoize `onKeyDown`
cee-chen Oct 1, 2023
b9feb3d
More `onKeyDown` cleanup
cee-chen Oct 1, 2023
f1b2ae2
[nit] Misc code organization
cee-chen Oct 1, 2023
98805e0
Update downstream snapshots
cee-chen Oct 1, 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
75 changes: 56 additions & 19 deletions src/components/popover/input_popover.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,25 +61,62 @@ describe('EuiPopover', () => {
cy.get('[data-popover-panel]').should('have.css', 'left', '200px');
});

it('correctly repositions the popover on input resize', () => {
cy.mount(
<div className="eui-textCenter">
<EuiInputPopover
{...props}
display="inline-block"
input={
<EuiTextArea rows={1} resize="horizontal" style={{ width: 150 }} />
}
>
Popover content
</EuiInputPopover>
</div>
);
cy.get('[data-popover-panel]').should('have.css', 'left', '175px');
cy.wait(100); // Wait a tick, otherwise Cypress returns a false positive
describe('on input resize', () => {
const initialWidth = 250;
const resizeProps = {
...props,
display: 'inline-block',
input: (
<EuiTextArea
rows={1}
resize="horizontal"
style={{ width: initialWidth }}
/>
),
};
const resizeInput = (width: number) => {
// Cypress doesn't seem to have a way to mimic manual dragging/resizing, so we'll do it programmatically
cy.get('textarea').then(($el) => ($el[0].style.width = `${width}px`));
};

it('repositions and resizes the popover to match the input', () => {
cy.mount(
<div className="eui-textCenter">
<EuiInputPopover {...resizeProps}>Popover content</EuiInputPopover>
</div>
);
cy.get('[data-popover-panel]')
.should('have.css', 'inline-size', '250px')
.should('have.css', 'left', '125px');

resizeInput(150);

cy.get('[data-popover-panel]')
.should('have.css', 'inline-size', '150px')
.should('have.css', 'left', '175px');
});

it('repositions the popover even when the popover width does not change', () => {
cy.mount(
<div className="eui-textCenter">
<EuiInputPopover
{...resizeProps}
panelMinWidth={initialWidth}
anchorPosition="downRight"
>
Popover content
</EuiInputPopover>
</div>
);
cy.get('[data-popover-panel]')
.should('have.css', 'inline-size', '250px')
.should('have.css', 'left', '125px');

resizeInput(100);

// Cypress doesn't seem to have a way to mimic manual dragging/resizing, so we'll do it programmatically
cy.get('textarea').then(($el) => ($el[0].style.width = '500px'));
cy.get('[data-popover-panel]').should('have.css', 'left', '50px');
cy.get('[data-popover-panel]')
.should('have.css', 'inline-size', '250px')
.should('have.css', 'left', '50px');
});
});
});
56 changes: 25 additions & 31 deletions src/components/popover/input_popover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import React, {
useState,
useEffect,
useCallback,
useMemo,
useRef,
} from 'react';
import { css } from '@emotion/react';
Expand All @@ -21,7 +22,7 @@ import { tabbable, FocusableElement } from 'tabbable';
import { logicalCSS } from '../../global_styling';
import { keys, useCombinedRefs, useEuiTheme } from '../../services';
import { CommonProps } from '../common';
import { EuiResizeObserver } from '../observer/resize_observer';
import { useResizeObserver } from '../observer/resize_observer';
import { EuiFocusTrap } from '../focus_trap';
import { euiFormVariables } from '../form/form.styles';

Expand Down Expand Up @@ -65,40 +66,37 @@ export const EuiInputPopover: FunctionComponent<EuiInputPopoverProps> = ({
}) => {
const euiThemeContext = useEuiTheme();
const [inputEl, setInputEl] = useState<HTMLElement | null>(null);
const [inputElWidth, setInputElWidth] = useState<number>();
const [panelEl, setPanelEl] = useState<HTMLElement | null>(null);
const popoverClassRef = useRef<EuiPopover | null>(null);

const inputRef = useCombinedRefs([setInputEl, _inputRef]);
const panelRef = useCombinedRefs([setPanelEl, _panelRef]);

const setPanelWidth = useCallback(
(width?: number) => {
if (panelEl && (!!inputElWidth || !!width)) {
const newWidth = !!width ? width : inputElWidth;
const widthToSet =
newWidth && newWidth > panelMinWidth ? newWidth : panelMinWidth;
/**
* Sizing/width logic
*/

const inputWidth = useResizeObserver(inputEl, 'width').width;

const panelWidth = useMemo(() => {
return inputWidth < panelMinWidth ? panelMinWidth : inputWidth;
}, [panelMinWidth, inputWidth]);
Comment on lines +90 to +94
Copy link
Contributor

Choose a reason for hiding this comment

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

[Attaching to the closest block of code] This is a lot easier to understand (especially with the new comments)

Copy link
Member Author

Choose a reason for hiding this comment

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

Music to my ears. Thank you so much Bree!

Also, for some context as to what I was cursing about & struggling with that I mentioned earlier today - it took me way too long to figure out what the tabbable logic was trying to accomplish and how to QA it (it really seems, functionally, to only affect the EuiAutoRefresh component). Which is why writing tests and leaving good code comments is so important :)


panelEl.style.width = `${widthToSet}px`;
onPanelResize?.(widthToSet);
}
},
[panelEl, inputElWidth, onPanelResize, panelMinWidth]
);
const onResize = useCallback(() => {
if (inputEl) {
const width = inputEl.getBoundingClientRect().width;
setInputElWidth(width);
setPanelWidth(width);
popoverClassRef.current?.positionPopoverFluid();
}
}, [inputEl, setPanelWidth]);
useEffect(() => {
onResize();
}, [onResize]);
if (panelEl) {
// We have to modify the popover panel DOM node directly instead of using
// `panelStyle`, as there's some weird positioning bugs on resize otherwise
panelEl.style.inlineSize = `${panelWidth}px`;
}
}, [panelEl, panelWidth]);

useEffect(() => {
setPanelWidth();
}, [setPanelWidth]);
// This fires on all input width changes regardless of minimum size, because on
// right/center anchored popovers, the input width affects the position of the popover
if (panelEl) {
popoverClassRef.current?.positionPopoverFluid();
}
}, [inputWidth, panelEl]);

const onKeyDown = (event: React.KeyboardEvent) => {
if (panelEl && event.key === keys.TAB) {
Expand Down Expand Up @@ -127,11 +125,7 @@ export const EuiInputPopover: FunctionComponent<EuiInputPopoverProps> = ({
css={css(fullWidth ? undefined : logicalCSS('max-width', form.maxWidth))}
repositionToCrossAxis={false}
ownFocus={false}
button={
<EuiResizeObserver onResize={onResize}>
{(resizeRef) => <div ref={resizeRef}>{input}</div>}
</EuiResizeObserver>
}
button={input}
buttonRef={inputRef}
panelRef={panelRef}
className={classes}
Expand Down