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

[EuiResizableButton] Add new indicator style prop #7455

Merged
merged 11 commits into from
Jan 12, 2024

Conversation

cee-chen
Copy link
Contributor

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

Summary

closes #7447

This PR updates <EuiResizableButton /> to accept a indicator={handle|border} prop, which allows rendering either the default grab handle or a thin subdued border. It also updates <EuiFlyoutResizable /> to use this new prop.

Screencaps

resizable_indicator_storybook

resizable_indicator

QA

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA
  • Code quality checklist
    • Tested Storybook
      - [ ] 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
    - [ ] Updated the Figma library counterpart - Resizable container doesn't appear to be in our Figma library

causes a bunch of indentation/whitespace diffs, hence the separate commit
- there's a *lot* of CSS in there that only applies to the grab handle

- requires an extra horizontal/vertical key, but IMO this is worth it and easier to understand when reading the CSS
- include plain border design

- remove references to grab `icon` (it's not really an icon), refer to `handle` instead

+ update downstream snapshots
+ update storybook w/ more production-like UI
@cee-chen
Copy link
Contributor Author

cee-chen commented Jan 9, 2024

@MichaelMarcialis Do you mind taking a look at this and confirming if it looks good to you / if Discover would be able to use it?

@cee-chen cee-chen marked this pull request as ready for review January 9, 2024 23:40
@cee-chen cee-chen requested a review from a team as a code owner January 9, 2024 23:40
Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

👍 LGTM! I went through the manual QA with four evergreen browsers and quick verified the screen reader announcement for horizontal and vertical scroll handles.

Copy link
Member

@JasonStoltz JasonStoltz left a comment

Choose a reason for hiding this comment

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

Code LGTM

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

Wow, that was very fast! Please forgive me for the late review. Amazing work as usual, @cee-chen. This addition looks fantastic.

I imagine that this would indeed allow us to replace some of the current implementations of the resizable buttons in Discover (such as the one in the unified search interface and the ones dividing other sections of the app that are currently utilizing CSS overrides to achieve this style). CCing @stratoula and @jughosta, in case they wanted to take a look as well.

The only comment I have on the PR is regarding the prop name and values. Is there any concern that the new showIndicator={false} prop may give the false impression that there will be no affordance for the resizeable button whatsoever (when in reality, affordance is still provided on hover and focus, and is just displayed as a standard border rather than resembling one of our grab icons by default)? Would changing the prop name and it's values to something like handle="icon" (which would be the default) and handle="border" be easier to understand? I'll defer to ya'll on this, but wanted to bring it up for discussion.

In any case, that alone isn't worth holding you up over. Regardless of what direction you wish to go with the prop name and values, I'll go ahead and approve now. Thanks again!

@cee-chen
Copy link
Contributor Author

cee-chen commented Jan 11, 2024

Ooo, changing the API name to be clearer is a great call - thanks Michael!! I'll make that change here shortly!

To match the existing alignIndicator prop, I'm thinking indicator={handle|border} - WDYT? The prop itself having handle in the name is a bit confusing to me personally. I see a handle as something small like a doorknob, and not like a whole border. 🤔

@stratoula
Copy link
Contributor

This is fantastic! In ES|QL editor I am already using the EUI one but I can use this prop to make it closer to what @MichaelMarcialis wants!!

@davismcphee
Copy link
Contributor

Hey team, thanks for adding this, and we'll definitely use it in Discover instead of overriding the CSS! One thing to note though is that we also override the blue shadow around the drag border, keeping just the dark blue highlighting on the border:
resize

This won't prevent us from using this new prop though of course, and only overriding the shadow (or doing whatever our design friends instruct us to 😁), just wanted to note it here as well.

@JasonStoltz
Copy link
Member

@davismcphee Thank you for the callout @davismcphee! My 2 cents, unless there's something super unique about Discover that requires the blue shadow, I'd vote to remove that override to keep consistent.

@cee-chen
Copy link
Contributor Author

cee-chen commented Jan 12, 2024

++ to what Jason said. I personally don't think removing the slight blue shade on focus/active is necessary, and it provides important visual context for mouse users with, e.g. low vision.

I asked @MichaelMarcialis about this in the originating issue (#7447 (comment)) and he didn't push back on it, which I assume means he's in total agreement 😆 (just kidding, please feel free to chime in Marcialis!)

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@davismcphee
Copy link
Contributor

Thanks both! Your points both make sense and I don't really have a strong opinion on it either way, so I'll defer to @andreadelrio and @MichaelMarcialis as the design leads on the Discover design refresh.

@MichaelMarcialis
Copy link
Contributor

To match the existing alignIndicator prop, I'm thinking indicator={handle|border} - WDYT? The prop itself having handle in the name is a bit confusing to me personally. I see a handle as something small like a doorknob, and not like a whole border. 🤔

I like it. Let's go with your proposed wording, @cee-chen.

++ to what Jason said. I personally don't think removing the slight blue shade on focus/active is necessary, and it provides important visual context for mouse users with, e.g. low vision.

I asked @MichaelMarcialis about this in the originating issue (#7447 (comment)) and he didn't push back on it, which I assume means he's in total agreement 😆 (just kidding, please feel free to chime in Marcialis!)

Oh yeah, I forgot to respond to that! Yeah, I'm good with keeping the light blue aura/shadow on focus and active states. That way the only difference between the two resizable buttons is whether it shows as a drag handle or a border. If we start encountering issues with the blue aura/shadow thing in our designs at a later date, we can revisit.

@cee-chen cee-chen changed the title [EuiResizableButton] Add new showIndicator prop [EuiResizableButton] Add new indicator style prop Jan 12, 2024
@cee-chen
Copy link
Contributor Author

Huzzah! Thanks y'all, this was a great discussion! I'm going to go ahead and merge in this PR, it should be in Kibana main sometime next week. I miiiight try to go ahead and remove/replace the custom CSS in Discover with this new prop as part of the next EUI upgrade, but no promises (if it turns out to be too tricky for me to handle). Feel free to shout Davis or Stratoula, if you have any issues using the new prop!

@cee-chen cee-chen merged commit 6fdfd2f into elastic:main Jan 12, 2024
7 checks passed
@cee-chen cee-chen deleted the resizable-button-showIndicator branch January 12, 2024 18:26
cee-chen added a commit to elastic/kibana that referenced this pull request Jan 24, 2024
`v92.0.0-backport.0`⏩ `v92.1.1`

---

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

**Bug fixes**

- Minor `EuiDataGrid` cell performance fixes
([#7465](elastic/eui#7465))

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

- Updated `EuiResizableButton` to allow customizing the `indicator`
style with either `handle` (default) or `border`
([#7455](elastic/eui#7455))
- Enhanced `EuiResizableContainer` to preserve the drag/resize event
when the user's mouse leaves the parent container and re-enters
([#7456](elastic/eui#7456))

**Bug fixes**

- Fixed an `EuiTreeView` JSX Typescript error
([#7452](elastic/eui#7452))
- Fixed a color console warning being generated by disabled `EuiStep`s
([#7454](elastic/eui#7454))

**Accessibility**

- `EuiDataGrid`'s keyboard/screenreader experience has been tweaked to
be more consistent for varying complex data:
([#7448](elastic/eui#7448))
- Headers are now always navigable by arrow key, regardless of whether
the header cells contain interactive content
- Non-expandable cells containing any amount of interactive content now
must be entered via Enter or F2 keypress
  - Expandable cells continue to be toggled via Enter or F2 keypress
- `EuiDataGrid` now provides a direct screen reader hint for Enter key
behavior for expandable & interactive cells
([#7448](elastic/eui#7448))
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
`v92.0.0-backport.0`⏩ `v92.1.1`

---

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

**Bug fixes**

- Minor `EuiDataGrid` cell performance fixes
([elastic#7465](elastic/eui#7465))

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

- Updated `EuiResizableButton` to allow customizing the `indicator`
style with either `handle` (default) or `border`
([elastic#7455](elastic/eui#7455))
- Enhanced `EuiResizableContainer` to preserve the drag/resize event
when the user's mouse leaves the parent container and re-enters
([elastic#7456](elastic/eui#7456))

**Bug fixes**

- Fixed an `EuiTreeView` JSX Typescript error
([elastic#7452](elastic/eui#7452))
- Fixed a color console warning being generated by disabled `EuiStep`s
([elastic#7454](elastic/eui#7454))

**Accessibility**

- `EuiDataGrid`'s keyboard/screenreader experience has been tweaked to
be more consistent for varying complex data:
([elastic#7448](elastic/eui#7448))
- Headers are now always navigable by arrow key, regardless of whether
the header cells contain interactive content
- Non-expandable cells containing any amount of interactive content now
must be entered via Enter or F2 keypress
  - Expandable cells continue to be toggled via Enter or F2 keypress
- `EuiDataGrid` now provides a direct screen reader hint for Enter key
behavior for expandable & interactive cells
([elastic#7448](elastic/eui#7448))
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.

[EuiResizableButton] Prop to switch between drag handle icon or simple border
8 participants