use react-virtualized to virtualize EuiComboBox options list #670

nreese · 2018-04-13T18:37:38Z

fixes #651

paired with @cjcenizal to convert EuiComboBox rendering to use react-virtualized.

Rendering long lists was really slow because each list item was added to the DOM. With virtualization, only the list items that are visible in the EuiComboBoxOptionsList panel are in the DOM and there is great performance for large lists.

nreese · 2018-04-13T21:15:21Z

@cjcenizal I think I have gone about as far as I can with this. Remaining questions/actions

rowHeight is getting set to a hard coded value. This needs to be determined dynamically. Update: Add rowHeight property to support renderOption that renders custom height values.
Truncate labels that wrap when virtualization is used.
Should virtualization be configurable via a prop or should it always be used? By using virtualization, you loose the ability to have rows with different heights. Maybe this is more important for some users because then no labels get truncated. Update: No, visualization should not be configurable via a prop until there is a good reason to do so.
Does a prop need to be added for rendering group title? If renderOption creates rows of height 40, then users will need a function to create group title rows of the same height. Update: Not needed yet.
Fix keyboard navigation, i.e. using the up and down arrows to go through the list of options

cjcenizal

This is looking so awesome! I created nreese#1 to address the text truncation and do some minor cleanup. It should address some of the comments I left below.

I think virtualization should be always-on by default. This will keep the interface simpler so the consumer doesn't need to worry about this. And if it turns out we should allow the consumer to control this then it's easier to add a prop than to remove one.

Does a prop need to be added for rendering a group title?

I don't have the cycles to think this through, so for now I'm comfortable punting on this. My gut tells me we should see what kinds of use cases people have for rendering options and virtualization and then make a decision once we have more info. Will you be using custom rendering in your use case?

We also need to add another checkbox to the list of to-do's:

Fix keyboard navigation, i.e. using the up and down arrows to go through the list of options

It works surprisingly well when just using the down arrow key, but it doesn't wrap to the end when you use the up arrow key to go past the first option.

Also, somehow I got this error by using the arrow keys to navigating and then clicking on the input:

cjcenizal · 2018-04-16T04:08:50Z

src/components/combo_box/combo_box.js

@@ -485,6 +490,7 @@ export class EuiComboBox extends Component {
 let optionsList;

 if (!noSuggestions && isListOpen) {
+ console.log("width", width);


Leftover console.log.

cjcenizal · 2018-04-16T04:10:14Z

src/components/combo_box/combo_box_options_list/_combo_box_options_list.scss

+ overflow: hidden;
+ }
+
+ .ReactVirtualized__List {


Can we put this inside of the .euiComboBoxOptionsList class so these styles are scoped to this component?

.euiComboBoxOptionsList { @include euiFormControlSize; box-sizing: content-box; /* 1 */ margin-left: -1px; /* 1 */ z-index: $euiZComboBox; position: absolute; /* 2 */ top: 0; /* 2 */ .ReactVirtualized__List { @include euiScrollBar; } }

cjcenizal · 2018-04-16T04:11:11Z

src/components/combo_box/combo_box_options_list/_combo_box_title.scss

 .euiComboBoxTitle {
 font-size: $euiFontSizeXS;
- padding: $euiSizeXS $euiSizeS $euiSizeXS 0;
+ padding: ($euiSizeXS + $euiSizeS - 1px) $euiSizeS $euiSizeXS 0; /* 1 */


Can we tweak this to be:

padding: ($euiSizeXS + $euiSizeS - 1px) $euiSizeS $euiSizeXS;

And then can we edit .euiComboBoxOption so that its padding (line 3 of _combo_box_option.scss) is this:

padding: $euiSizeXS $euiSizeS $euiSizeXS #{$euiSizeM + $euiSizeXS};

cjcenizal · 2018-04-16T04:11:45Z

src/components/combo_box/combo_box_options_list/combo_box_options_list.js

- optionsList.push(renderedOption);
- });
+ const optionHeight = 27; // TODO dynamically figure this out
+ const numVisiableOptions = matchingOptions.length < 7 ? matchingOptions.length : 7;


Minor typo, this should be numVisibleOptions

cjcenizal · 2018-04-16T04:12:44Z

src/components/combo_box/matching_options.js

+ });
+ if (matchingOptionsForGroup.length > 0) {
+ // Add option for group label
+ matchingOptions.push({ label: option.label, isGroupLabelOption: true });


Nice improvement!

nreese · 2018-04-16T12:51:03Z

we should see what kinds of use cases people have for rendering options and virtualization and then make a decision once we have more info. Will you be using custom rendering in your use case?

No, my use case will not use custom rendering.

cjcenizal

Nice! Focus tracking looks great. I found a couple more things and submitted nreese#2 to address them. Once we merge it I think we're good to go.

cjcenizal · 2018-04-16T23:16:25Z

src/components/combo_box/combo_box.js

 };

 clearActiveOption = () => {
- this.activeOptionIndex = undefined;
+ this.state.activeOptionIndex = undefined;


I think we want to call setState here, right?

cjcenizal

🌊 Rad!! I only had a few comments, all minor.

cjcenizal · 2018-04-17T01:39:10Z

src-docs/src/views/combo_box/combo_box_example.js

+ code: virtualizedHtml,
+ }],
+ text: (
+ <p>


Can we tweak this to fix some spacing and a typo:

<p> <EuiCode>EuiComboBoxList</EuiCode> uses <Link to="https://github.com/bvaughn/react-virtualized">react-virtualized</Link>{' '} to only render visible options to be super fast no matter how many options there are. </p>

cjcenizal · 2018-04-17T01:39:54Z

src-docs/src/views/combo_box/combo_box_example.js

- <EuiCode>option</EuiCode> object to store metadata about the option for use in this callback.
- </p>
+ <Fragment>
+ <p>


This will fix some more spacing issues:

<p> You can provide a <EuiCode>renderOption</EuiCode> prop which will accept <EuiCode>option</EuiCode>{' '} and <EuiCode>searchValue</EuiCode> arguments. Use the <EuiCode>value</EuiCode> prop of the{' '} <EuiCode>option</EuiCode> object to store metadata about the option for use in this callback. </p>

cjcenizal · 2018-04-17T01:41:24Z

src/components/combo_box/combo_box.js

@@ -149,7 +151,7 @@ export class EuiComboBox extends Component {
 tabbableItems[comboBoxIndex + amount].focus();
 };

- incrementActiveOptionIndex = amount => {
+ incrementActiveOptionIndex = _.throttle(amount => {


Can we import the throttle module?

import { throttle } from 'lodash';

cjcenizal · 2018-04-17T01:42:09Z

src/components/combo_box/combo_box.js

+ this.setState({
+ activeOptionIndex: nextActiveOptionIndex,
+ });
+ }, 200);


200 feels a bit sluggish to me -- 100 feels snappier, can we use that?

with 100 I was still getting the keydown to be faster than the rendering and I could hold the key down and the focus option would not keep up

snide

Only stuff I caught in here was that disabled items shouldn't be focusable (this is a regression from master)

And that there's a minor hitch in the truncation in the custom example. Likely though that would be adjusted on the custom element itself.

cjcenizal · 2018-04-17T23:32:27Z

@snide I made the options focusable so that screen readers can pick them up and read them out as "disabled". Otherwise someone using a screen reader won't know that the option is in the list. This can be a little weird for sighted users but I think it's worth it for the accessibility gain. What do you think?

You're right about the custom rendering example; it would have to be handled by the consumer. I thought about updating the example to truncate the text, but it would mean introducing a custom class for a single example and I wasn't sure it was worth it. We can update the example text to provide guidance on this though.

snide · 2018-04-18T16:53:43Z

I think it shouldn't be focusable, for similar reasons that @cchaos had the last time this came up in #653 (comment). I think its safest to follow what the browser provides for disabled states on button tags.

I don't feel strongly enough on it to prevent this PR from being merged though since it's a great improvement.

…d title attribute to options for usability. (#1)

* Call setState instead of setting activeOptionIndex directly. * Clear activeOptionIndex when you click the input. * Prevent a lot of input from overflowing the container. * Allow disabled options to be focused but not selected.

…g UI out of sync

cjcenizal · 2018-04-18T17:17:10Z

I think it's OK to keep it in this case because it replicates the behavior of a select element in terms of keyboard accessibility. Here's a capture of using the example on w3schools:

The only aspect which is odd is that you can focus the disabled item. Sighted users will have the "not-allowed" cursor and dimmed visual state to signal that the item can't be selected, so I think it's a worthwhile tradeoff.

Let's keep it in for now and get @aphelionz's input on this at some point. Maybe he knows of a better solution.

This reverts commit 47fa3ef.

nreese added the work in progress label Apr 13, 2018

cjcenizal reviewed Apr 16, 2018

View reviewed changes

nreese force-pushed the virtualized_combo_box branch from 88c1476 to 1a45da9 Compare April 17, 2018 01:24

nreese requested review from snide and cjcenizal April 17, 2018 01:33

nreese removed the work in progress label Apr 17, 2018

cjcenizal approved these changes Apr 17, 2018

View reviewed changes

snide approved these changes Apr 17, 2018

View reviewed changes

nreese and others added 14 commits April 18, 2018 11:11

use react-virtualized to virtualize combo box options list

48d13c8

use smaller width and height

c0722c6

include group label in matching options list

09730ba

add better text for example description

9fa163f

dynamically set width and height

7380a41

Massage group title padding. Truncate text instead of wrapping it. Ad…

fd0214a

…d title attribute to options for usability. (#1)

remove console.log and fix spelling

b024db6

fix problems with settig focus on active option

d18690e

more keyboard accessiblity work

e417a18

Combo box focus state and text overflow (#2)

85550ff

* Call setState instead of setting activeOptionIndex directly. * Clear activeOptionIndex when you click the input. * Prevent a lot of input from overflowing the container. * Allow disabled options to be focused but not selected.

add throttle to incrementActiveOptionIndex to avoid keypresses gettin…

68c9826

…g UI out of sync

rowHeight prop

fde8a5b

remove unneeded const

f4426ab

fix spacing in example text, fix lodash import

c355fed

nreese force-pushed the virtualized_combo_box branch from 319c7e4 to c355fed Compare April 18, 2018 17:11

skip disabled options when using keyboard

47fa3ef

Revert "skip disabled options when using keyboard"

cc4451d

This reverts commit 47fa3ef.

nreese merged commit cd12e5b into elastic:master Apr 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use react-virtualized to virtualize EuiComboBox options list #670

use react-virtualized to virtualize EuiComboBox options list #670

nreese commented Apr 13, 2018 •

edited

Loading

nreese commented Apr 13, 2018 •

edited

Loading

cjcenizal left a comment

cjcenizal Apr 16, 2018

cjcenizal Apr 16, 2018

cjcenizal Apr 16, 2018

cjcenizal Apr 16, 2018

cjcenizal Apr 16, 2018

nreese commented Apr 16, 2018

cjcenizal left a comment

cjcenizal Apr 16, 2018

cjcenizal left a comment

cjcenizal Apr 17, 2018

cjcenizal Apr 17, 2018

cjcenizal Apr 17, 2018

cjcenizal Apr 17, 2018

nreese Apr 17, 2018

snide left a comment

cjcenizal commented Apr 17, 2018

snide commented Apr 18, 2018

cjcenizal commented Apr 18, 2018

use react-virtualized to virtualize EuiComboBox options list #670

use react-virtualized to virtualize EuiComboBox options list #670

Conversation

nreese commented Apr 13, 2018 • edited Loading

nreese commented Apr 13, 2018 • edited Loading

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nreese commented Apr 16, 2018

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snide left a comment

Choose a reason for hiding this comment

cjcenizal commented Apr 17, 2018

snide commented Apr 18, 2018

cjcenizal commented Apr 18, 2018

nreese commented Apr 13, 2018 •

edited

Loading

nreese commented Apr 13, 2018 •

edited

Loading