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

[EuiFlyoutResizable] Add optional onResize callback #7464

Merged
merged 4 commits into from
Jan 23, 2024
Merged

[EuiFlyoutResizable] Add optional onResize callback #7464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cf3d9ac
[REVERT ME] Docs QA testing
cee-chen Jan 16, 2024
46ff239
Add `onResize` prop callback
cee-chen Jan 16, 2024
7407b4f
changelog
cee-chen Jan 16, 2024
866f74e
Revert "[REVERT ME] Docs QA testing"
cee-chen Jan 23, 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
1 change: 1 addition & 0 deletions changelogs/upcoming/7464.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Updated `EuiFlyoutResizable` with new optional `onResize` callback
30 changes: 30 additions & 0 deletions src/components/flyout/flyout_resizable.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,36 @@ describe('EuiFlyoutResizable', () => {
cy.get('.euiFlyout').should('have.css', 'inline-size', '400px');
};
});

it('calls the optional onResize callback on mouseup and keyboard events only', () => {
const onResize = cy.stub();
cy.mount(
<EuiFlyoutResizable onClose={onClose} size={800} onResize={onResize} />
);

cy.get('[data-test-subj="euiResizableButton"]')
.trigger('mousedown', { clientX: 400 })
.trigger('mousemove', { clientX: 600 })
.then(() => {
expect(onResize).not.have.been.called;
});
cy.get('[data-test-subj="euiResizableButton"]')
.trigger('mouseup')
.then(() => {
expect(onResize.callCount).to.eql(1);
expect(onResize).to.have.been.calledWith(600);
});

cy.get('[data-test-subj="euiResizableButton"]').focus();
cy.realPress('ArrowRight').then(() => {
expect(onResize.callCount).to.eql(2);
expect(onResize).to.have.been.calledWith(590);
});
cy.realPress('ArrowLeft').then(() => {
expect(onResize.callCount).to.eql(3);
expect(onResize.lastCall.args).to.eql([600]);
});
});
});

describe('push flyouts', () => {
Expand Down
19 changes: 19 additions & 0 deletions src/components/flyout/flyout_resizable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ import { euiFlyoutResizableButtonStyles } from './flyout_resizable.styles';
export type EuiFlyoutResizableProps = Omit<EuiFlyoutProps, 'maxWidth'> & {
maxWidth?: number;
minWidth?: number;
/**
* Optional callback that fires on user resize with the new flyout width
*/
onResize?: (width: number) => void;
};

export const EuiFlyoutResizable = forwardRef(
Expand All @@ -33,6 +37,7 @@ export const EuiFlyoutResizable = forwardRef(
size,
maxWidth,
minWidth = 200,
onResize,
side = 'right',
type = 'overlay',
children,
Expand All @@ -56,11 +61,13 @@ export const EuiFlyoutResizable = forwardRef(
);

const [flyoutWidth, setFlyoutWidth] = useState(0);
const [callOnResize, setCallOnResize] = useState(false);

// Must use state for the flyout ref in order for the useEffect to be correctly called after render
const [flyoutRef, setFlyoutRef] = useState<HTMLElement | null>(null);
const setRefs = useCombinedRefs([setFlyoutRef, ref]);
useEffect(() => {
setCallOnResize(false); // Don't call `onResize` for non-user width changes
setFlyoutWidth(
flyoutRef ? getFlyoutMinMaxWidth(flyoutRef.offsetWidth) : 0
);
Expand Down Expand Up @@ -92,6 +99,7 @@ export const EuiFlyoutResizable = forwardRef(
);

const onMouseUp = useCallback(() => {
setCallOnResize(true);
initialMouseX.current = 0;

window.removeEventListener('mousemove', onMouseMove);
Expand All @@ -102,6 +110,7 @@ export const EuiFlyoutResizable = forwardRef(

const onMouseDown = useCallback(
(e: React.MouseEvent | React.TouchEvent) => {
setCallOnResize(false);
initialMouseX.current = getPosition(e, true);
initialWidth.current = flyoutRef?.offsetWidth ?? 0;

Expand All @@ -117,6 +126,7 @@ export const EuiFlyoutResizable = forwardRef(

const onKeyDown = useCallback(
(e: React.KeyboardEvent) => {
setCallOnResize(true);
const KEYBOARD_OFFSET = 10;

switch (e.key) {
Expand All @@ -136,6 +146,15 @@ export const EuiFlyoutResizable = forwardRef(
[getFlyoutMinMaxWidth, direction]
);

// To reduce unnecessary calls, only fire onResize callback:
// 1. After initial mount / on user width change events only
// 2. If not currently mouse dragging
Comment on lines +149 to +151
Copy link
Member

@tkajtoch tkajtoch Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I can see a need for receiving continuous updates during mouse dragging, and I think we may eventually have to implement it. We usually let users deal with debouncing these calls when necessary, so maybe just do the same thing here and keep it consistent? Alternatively, we could provide a configuration prop like onResizeUpdateFrequency: 'once' | 'always' but that sounds like a new pattern we'd need to introduce to other places as well.

Copy link
Member Author

@cee-chen cee-chen Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That's fair, but I think I'd rather wait until it's requested? TBH I kind of felt that the onResizeStart and onResizeEnd callbacks/contributions to EuiResizableContainer were stopgaps for this exact reason - consumers/devs only wanted the final width at the end of resize, not on mouse drag.

a new pattern we'd need to introduce to other places as well

FWIW EuiFlyoutResizable is already different from EuiResizableContainer in this regard, in that its width(s) are not controllable unlike EuiResizableContainer. I don't think it needs to be 1:1 on this, but if we feel differently we can change it - it's a beta component after all

Copy link
Member Author

@cee-chen cee-chen Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, why don't we double check with the dev who requested this feature! @mykolaharmash - for EuiFlyoutResizable's new onResize callback, would you want the updated width during the user drag event (potentially being called many times per second), or would you only want the final width after the user is done dragging?

Copy link

Choose a reason for hiding this comment

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

Hey folks! Sorry for a delayed response. For the usecase I had in mind (just saving the preferred size and restoring it later) having a single callback when the event end is totally fine. The only potential usecase I imagine where I'd need a continuous stream of events is when I'd want to some other layout or element on the page react to the updated flyout size, but it's only hypothetical, I don't have a realy need for this at the moment.

Thank you for implementing this 🙌 🙏

useEffect(() => {
if (callOnResize) {
onResize?.(flyoutWidth);
}
}, [onResize, callOnResize, flyoutWidth]);

return (
<EuiFlyout
{...rest}
Expand Down
Loading