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

[EuiSelectable] Support scrolling in non-virtualized lists #7609

Merged
merged 6 commits into from
Mar 22, 2024
Merged

[EuiSelectable] Support scrolling in non-virtualized lists #7609

merged 6 commits into from
Mar 22, 2024

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Mar 21, 2024 •
edited by mgadewoll
Loading

Summary

@jeramysoucy pointed out this missing functionality/feature request in the ability to turn off virtualization in EuiSelectable - it should continue to support scrolling as necessary. This is fairly trivial for EUI to do and it's not totally clear to me why it wasn't implemented in the first place when isVirtualized={false} was added as a prop, so I've gone ahead and added support for that in this PR.

Before After
screenshot screenshot

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
  • 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 commented Mar 21, 2024
View reviewed changes
src/components/selectable/selectable_list/_selectable_list.scss
@@ -20,6 +20,8 @@
.euiSelectableList__list {
@include euiOverflowShadow;
@include euiScrollBar;
overflow: auto;
position: relative; // Chrome/Edge loses its mind without this and doesn't render `mask-image` correctly
Copy link
Member Author

Choose a reason for hiding this comment

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

Without position: relative, Chrome/Edge does this:

TBH I don't really know why, I'm just grateful it's a fairly innocuous one-liner workaround instead of a major headache 🤷

@kibanamachine
Copy link

Preview staging links for this PR:

@cee-chen cee-chen marked this pull request as ready for review March 21, 2024 19:31
@cee-chen cee-chen requested a review from a team as a code owner March 21, 2024 19:31
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

mgadewoll
mgadewoll reviewed Mar 22, 2024
View reviewed changes
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.

I noticed a small difference between isVirtualized=true and isVirtualized=false items. (seen on Chrome and Edge only)

Screen.Recording.2024-03-22.at.11.27.41.mov

From what I can see, there is a will-change: transform on the euiSelectableList__list when isVirtualized=true which is not applied for isVirtualized=false. Adding it results in the same output.

src/components/selectable/selectable_list/selectable_list.tsx Show resolved Hide resolved
@cee-chen
Copy link
Member Author

I'm tempted to leave the will-change: transform thing alone personally. I don't think the minute pixel difference is worth the tradeoff in terms of CSS performance impact, and will additionally depend on the browser or zoom level in any case. Also, devs aren't going to be switching back and forth between virtualized/non-virtualized, so I don't think end-users will really notice it.

@mgadewoll
Copy link
Contributor

I'm tempted to leave the will-change: transform thing alone personally. I don't think the minute pixel difference is worth the tradeoff in terms of CSS performance impact, and will additionally depend on the browser or zoom level in any case. Also, devs aren't going to be switching back and forth between virtualized/non-virtualized, so I don't think end-users will really notice it.

I agree on your points. Given that isVirtualized=false already has a bigger effect on performance due to more DOM elements and the difference is tiny tiny, it should be fine to leave it.
I just thought it best to raise it anyway in case we do want to/need to ensure parity here 🙂

mgadewoll
mgadewoll approved these changes Mar 22, 2024
View reviewed changes
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.

🚢 🐈‍⬛ LGTM!

@cee-chen
Copy link
Member Author

Thanks as always Lene!

@cee-chen cee-chen merged commit 9df1914 into elastic:main Mar 22, 2024
8 checks passed
@cee-chen cee-chen deleted the selectable/unvirtualized-scrolling branch March 22, 2024 15:52
@cee-chen cee-chen mentioned this pull request Mar 25, 2024
Merged
cee-chen added a commit to elastic/kibana that referenced this pull request Mar 26, 2024
@cee-chen
Upgrade EUI to v93.5.1 (#179363)
8eb2fbd 
`v93.4.0` ⏩ `v93.5.1`

---

## [`v93.5.1`](https://github.com/elastic/eui/releases/v93.5.1)

**Bug fixes**

- Fixed unvirtualized `EuiSelectable`s to not cause Jest/jsdom errors on
active option change ([#7618](elastic/eui#7618))

## [`v93.5.0`](https://github.com/elastic/eui/releases/v93.5.0)

- `EuiHeaderLinks` now accepts a `children` render function that will be
passed a `closeMobilePopover` callback, allowing consumers to close the
mobile popover by its content
([#7603](elastic/eui#7603))
- Updated `EuiSelectable` to support scrolling list containers when
`listProps.isVirtualization` is set to `false`
([#7609](elastic/eui#7609))

**Bug fixes**

- Fixed `EuiIconTip`'s default `aria-label` text to be i18n tokenizable
([#7606](elastic/eui#7606))
- Fixed `EuiTextArea`'s CSS box model to no longer render a few extra
pixels of strut height
([#7607](elastic/eui#7607))

**Dependency updates**

- Updated `@types/refractor` to v3.4.0
([#7590](elastic/eui#7590))
- Updated `@types/lodash` to v4.14.202
([#7591](elastic/eui#7591))
- Removed `@types/resize-observer-browser` dependency. `ResizeObserver`
types should already be baked in to Typescript as of 4.2+
([#7592](elastic/eui#7592))
- Updated `classnames` to v2.5.1
([#7593](elastic/eui#7593))
- Updated `@types/numeral` to v2.0.5
([#7594](elastic/eui#7594))
- Updated `@types/react-window` to 1.8.8
([#7597](elastic/eui#7597))
- Updated `prop-types` to v15.18.1
([#7602](elastic/eui#7602))
- Removed `prop-types` as a peer dependency, per package recommendation
([#7602](elastic/eui#7602))

**Accessibility**

- `EuiIcons` no longer apply `aria-hidden` to empty icons, as long as a
valid title or label is provided to the icon. In particular, this is
intended to improve the accessibility of loading `EuiIconTip`s.
([#7606](elastic/eui#7606))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgadewoll mgadewoll mgadewoll approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cee-chen @kibanamachine @elasticmachine @mgadewoll