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] Do not extend resize click area past flyout for flyouts with no overlays #8010

Merged
merged 2 commits into from
Sep 9, 2024

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Sep 7, 2024

Summary

closes #7988

This PR fixes the scenario where the content to the left of the resizable flyout has a scrollbar (or any other clickable element that butts up against the flyout), and the placement of the resize border/indicator button prevents the thing it's overlaying from being clicked:

Before After

FYI that I kept the centered styling on flyouts with overlay masks, because those overlays will close the entire flyout on outside click which feels super disruptive - I felt the extra space there would be more beneficial UX-wise to help minimize that for users who attempt to resize and are a few pixels off.

QA

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
      - [ ] Checked in mobile
  • Docs site QA - N/A
  • Code quality checklist
    - [ ] Added or updated jest and cypress tests
  • 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
    • If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)

- flyouts with no overlay mask (push and ownFocus=false)

- the goal of this is primarily to avoid avoid intercepting clicks on scrollbars or other content outside the flyout

- keeping the centered behavior on flyouts w/ overlays, because those will close the entire flyout on outside click which is super disruptive, & we want to minimize that
@cee-chen cee-chen marked this pull request as ready for review September 7, 2024 00:32
@cee-chen cee-chen requested a review from a team as a code owner September 7, 2024 00:32
@cee-chen
Copy link
Member Author

cee-chen commented Sep 7, 2024

@MichaelMarcialis Could I get a designer's UX review on this? I'm not sure whether the choice to keep the overlay flyout center-aligned was the right one (see PR description for rationale) or if consistency is more important.

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

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

🚢 🐈‍⬛ Straight forward and clean implementation, LGTM! 🎉

@MichaelMarcialis
Copy link
Contributor

Thanks for the ping, @cee-chen. For EuiFlyout specifically, I think this solution works. While the push flyout is offset right and not centered, it still has the same amount of "grab zone" as it's overlay counterpart, which is the consistency that matters most in this case IMO.

That said, this issue has brought another question to mind: should we offer a prop to apply this kind of offset directly on the EuiResizableButton component as well? The reason I ask is because this issue where a scrollbar is obscured by the resize button/handle can also appear when using vanilla EuiResizableContainer components. It's actually visible in our doc examples currently. Thoughts?

CleanShot 2024-09-09 at 14 24 14

@cee-chen
Copy link
Member Author

cee-chen commented Sep 9, 2024

@MichaelMarcialis Yes, I totally agree we should add a fix/prop for it for EuiResizableContainer usage as well! I have a separate branch for that if that's okay, I'll loop you in once it's ready for review!

@cee-chen cee-chen merged commit d2afcab into elastic:main Sep 9, 2024
6 checks passed
@cee-chen cee-chen deleted the resizable-flyout-no-overlay branch September 9, 2024 20:50
@cee-chen
Copy link
Member Author

Follow-up PR: #8021

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.

[EuiFlyout] Make resizeIndicator configureable
4 participants