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.

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

[EuiFlyoutResizable] Add optional onResize callback #7464

Merged
merged 4 commits into from
Jan 23, 2024

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Jan 16, 2024

Summary

Per recent EUI feedback - adds an onResize callback where the latest flyout width is passed, which allows consumers to do things like, e.g. save user preference by storing the flyout width in localStorage

QA

  • Go to https://eui.elastic.co/pr_7464/#/layout/flyout#resizable-flyouts
  • Open your browser devtools and go to the console
  • Confirm that the flyout with is only logged to the console once you stop dragging and the flyout width is settled, and not at the beginning or middle of a drag
  • Confirm that the flyout with is logged to the console on every left/right arrow keypress

General checklist

  • Revert [REVERT ME] QA commit
  • Browser QA - N/A
  • Docs site QA
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.
      - [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer checklist - N/A

@cee-chen cee-chen marked this pull request as ready for review January 16, 2024 20:34
@cee-chen cee-chen requested a review from a team as a code owner January 16, 2024 20:34
@tkajtoch tkajtoch self-requested a review January 22, 2024 17:14
Comment on lines +149 to +151
// To reduce unnecessary calls, only fire onResize callback:
// 1. After initial mount / on user width change events only
// 2. If not currently mouse dragging
Copy link
Member

@tkajtoch tkajtoch Jan 22, 2024

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

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

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?

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 🙌 🙏

Copy link
Member

@tkajtoch tkajtoch left a comment

Choose a reason for hiding this comment

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

I'm totally fine leaving the current onResize behavior as is since this is a beta component. LGTM!

@cee-chen
Copy link
Member Author

Thanks Tomasz! @mykolaharmash, please feel free to drop a comment or open an issue if you need onResize to fire during the drag and not just at the end of it - we can continue tweaking this component as needed!

@cee-chen cee-chen enabled auto-merge (squash) January 23, 2024 19:45
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@cee-chen cee-chen merged commit a9d0873 into elastic:main Jan 23, 2024
7 checks passed
@cee-chen cee-chen deleted the flyout-resizable-onresize branch January 23, 2024 20:11
cee-chen added a commit to elastic/kibana that referenced this pull request Jan 30, 2024
`v92.1.1`⏩`v92.2.1`

---

## [`v92.2.1`](https://github.com/elastic/eui/releases/v92.2.1)

**Bug fixes**

- Removed unintentional i18n tokens in prior release that should not
have been exported

## [`v92.2.0`](https://github.com/elastic/eui/releases/v92.2.0)

- Updated `EuiFlyoutResizable` with new optional `onResize` callback
([#7464](elastic/eui#7464))

**Bug fixes**

- Fixed an issue in `EuiResizableContainer` where `onResizeEnd` could
become a stale closure when renders occured between resize start and
end, resulting in an outdated version of a consumer's `onResizeEnd`
callback being called
([#7468](elastic/eui#7468))
- Fixed `EuiTextArea` to correctly fire `onChange` callbacks on clear
button click ([#7473](elastic/eui#7473))
- Fixed `EuiContextMenu`'s panel titles & items to not show underlines
on hover for non-interactive elements
([#7474](elastic/eui#7474))

**Deprecations**

- Remove unused public `EuiHue` and `EuiSaturation` subcomponent
exports. Use the parent `EuiColorPicker` component instead
([#7460](elastic/eui#7460))
- Remove unused public `EuiCommentTimeline` subcomponent export. Use the
parent `EuiComment` or `EuiCommentList` components instead.
([#7467](elastic/eui#7467))
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
`v92.1.1`⏩`v92.2.1`

---

## [`v92.2.1`](https://github.com/elastic/eui/releases/v92.2.1)

**Bug fixes**

- Removed unintentional i18n tokens in prior release that should not
have been exported

## [`v92.2.0`](https://github.com/elastic/eui/releases/v92.2.0)

- Updated `EuiFlyoutResizable` with new optional `onResize` callback
([elastic#7464](elastic/eui#7464))

**Bug fixes**

- Fixed an issue in `EuiResizableContainer` where `onResizeEnd` could
become a stale closure when renders occured between resize start and
end, resulting in an outdated version of a consumer's `onResizeEnd`
callback being called
([elastic#7468](elastic/eui#7468))
- Fixed `EuiTextArea` to correctly fire `onChange` callbacks on clear
button click ([elastic#7473](elastic/eui#7473))
- Fixed `EuiContextMenu`'s panel titles & items to not show underlines
on hover for non-interactive elements
([elastic#7474](elastic/eui#7474))

**Deprecations**

- Remove unused public `EuiHue` and `EuiSaturation` subcomponent
exports. Use the parent `EuiColorPicker` component instead
([elastic#7460](elastic/eui#7460))
- Remove unused public `EuiCommentTimeline` subcomponent export. Use the
parent `EuiComment` or `EuiCommentList` components instead.
([elastic#7467](elastic/eui#7467))
fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024
`v92.1.1`⏩`v92.2.1`

---

## [`v92.2.1`](https://github.com/elastic/eui/releases/v92.2.1)

**Bug fixes**

- Removed unintentional i18n tokens in prior release that should not
have been exported

## [`v92.2.0`](https://github.com/elastic/eui/releases/v92.2.0)

- Updated `EuiFlyoutResizable` with new optional `onResize` callback
([elastic#7464](elastic/eui#7464))

**Bug fixes**

- Fixed an issue in `EuiResizableContainer` where `onResizeEnd` could
become a stale closure when renders occured between resize start and
end, resulting in an outdated version of a consumer's `onResizeEnd`
callback being called
([elastic#7468](elastic/eui#7468))
- Fixed `EuiTextArea` to correctly fire `onChange` callbacks on clear
button click ([elastic#7473](elastic/eui#7473))
- Fixed `EuiContextMenu`'s panel titles & items to not show underlines
on hover for non-interactive elements
([elastic#7474](elastic/eui#7474))

**Deprecations**

- Remove unused public `EuiHue` and `EuiSaturation` subcomponent
exports. Use the parent `EuiColorPicker` component instead
([elastic#7460](elastic/eui#7460))
- Remove unused public `EuiCommentTimeline` subcomponent export. Use the
parent `EuiComment` or `EuiCommentList` components instead.
([elastic#7467](elastic/eui#7467))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants