[EuiSuggest][EuiSelectable] Allow controlled search `value` prop by cee-chen · Pull Request #5658 · elastic/eui

cee-chen · 2022-02-23T20:18:04Z

Summary

closes #5644
closes #5651
closes #4341 - value, disabled, and placeholder can now all be passed without Typescript issues

Enhancements
- Added the ability for EuiSelectable and EuiSuggest to accept controlled value props
Bug fixes
- Fixed EuiSuggest not correctly passing props to the search input (53c328e)
- Fixed EuiSelectable incorrectly rendering the passed id prop on the listbox instead of the parent wrapper (809ec87)
  - Rendering the parent ID on the listbox is problematic because the listbox is conditionally rendered when searchable. If no results are returned, the ID disappears from the page, which isn't great developer experience.
- Fixed EuiSelectable to no longer call searchProps.onChange when list items are clicked (b969621)
- Fixed EuiSelectable not respecting searchProps.inputRef (514b8f9)
Breaking changes
- Removed EuiSelectable's searchProps.onSearch prop (since Enter keypresses do not trigger a search callback) - use searchProps.onChange instead
- Renamed EuiSuggest's onInputChange and onSearchChange callbacks to onInput/onSearch respectively, for consistency with our existing callback naming conventions
- Removed EuiSuggest's isLoading prop - use status.loading instead
- Removed the incremental prop from EuiSuggest and EuiSelectable's searchProps
Refactors
- EuiSelectableSearch was converted from a class component to a functional component, and no longer maintains its own internal searchValue state (but inherits it from the parent EuiSelectable, which already maintains said state)

If possible, I recommend following along by commit.

QA

EuiSuggest
- Go to http://localhost:8030/#/forms/suggest
- Confirm the first example loads in with the field value, but still works as normal/expected when the user types or backspaces into the search input
- Go to the last example on the page (no controlled value prop) and type into the search input. Confirm that still works as normal/expected
EuiSelectable
- Go to http://localhost:8030/#/forms/selectable#searchable
- Confirm the Searchable example loads in with a t value, but still works as normal/expected when the user types or backspaces into the search input
- Go to the last example on the page (no controlled value prop) and type into the search input. Confirm that still works as normal/expected

Checklist

~~- [ ] Check against all themes for compatibility 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~~

Props have proper autodocs and playground toggles

- [ ] Added documentation
~~- [ ] Checked Code Sandbox works for any docs examples~~

Added or updated jest and cypress tests
Checked for breaking changes and labeled appropriately
A changelog entry exists and is marked appropriately

cee-chen · 2022-02-23T20:23:12Z

src/components/suggest/suggest.tsx

          onFocusBadge: false,
          paddingSize: 'none',
          isVirtualized,
-          ...rest,


Note: EuiSuggest previously passed ...rest to its suggest input

eui/src/components/suggest/suggest.tsx

Lines 54 to 61 in c85913b

const suggestInput = (

<EuiSuggestInput

status={status}

tooltipContent={tooltipContent}

append={append}

suggestions={suggestionList}

{...rest}

/>

so this PR moves ...rest from listProps to searchProps to maintain previous parity

👍 Makes much more sense. Can we also then add a listProps prop that can be passed down?

We definitely could! My only hesitation would be increasing the scope of this PR. As far as I can tell the EuiSuggest component didn't have a way to significantly customize the dropdown list before the EuiSelectable refactor. I'm good with adding it though if we're all OK with it and don't have any other larger API concerns/refactors

@thompsongl just wanted to check in with you on this before I added it in, are you good with adding listProps as a new prop to EuiSuggest? (or does it feel weird that there's a listProps but not a searchProps? Am I overthinking this? 🧠)

or does it feel weird that there's a listProps but not a searchProps? Am I overthinking this?

I think this the right question. I'd prefer opening an issue to propose adding both, rather than introducing the change in this PR. As we've learned, smaller changesets are better for these components 🙈

I'm good with that - I think that enhancement can wait until later, whereas a few folks now have been directly affected by the value bug, so I'd rather get this PR in sooner rather than later.

cee-chen · 2022-02-23T20:30:43Z

src/components/selectable/selectable_search/selectable_search.tsx

+      className={classes}
+      placeholder={placeholder}
+      value={value}
+      onChange={onChange}


This switch from onSearch to onChange is necessary because for whatever reason, EuiFieldSearch's underlying onSearch does not trigger correctly when value is passed, but onChange does.

I figured this change is mostly safe and not breaking because EuiSelectable (and its consuming applications) primarily rely on incremental search and are already overriding the Enter key presses to toggle list items. Additionally, EuiFieldSearch's docs themselves say to prefer onChange over onSearch if not using the enter key.

src/components/suggest/suggest.tsx

src/components/selectable/selectable.test.tsx

kibanamachine · 2022-02-23T21:05:47Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5658/

kibanamachine · 2022-02-23T22:15:44Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5658/

- to make it easier to test missing props

…st example - to make it easier to test behavior - could also be left in to serve as a controlled example 🤷

Some of these were confusing to me when they showed up in the playground, some of these straight up don't work or break EuiSelectable behavior when passed, and some should be removed for API consistency

- to match previous behavior as well as handle EuiFieldSearch props

…hild ID - we should be passing the direct ID prop to the parent wrapper, not to the `ul` listbox, which previously had a `_listbox` suffix

- While still supporting `defaultValue` or when consumers do not pass `value` - This happens by retaining our passed `onSearchChange`->`onChange` state callback while adding `getDerivedStateFromProps` logic for the `value` prop + clean up EuiSelectableSearch to not also repetitively maintain its own internal searchValue state - since we're storing it at the top EuiSelectable level, we can just maintain it in 1 place and pass it down to children + remove `defaultValue` from being passed - React will throw an error if you attempt to pass both `value` and `defaultValue` + switch EuiFieldSearch prop `onSearch` to `onChange` - unfortunately onSearch doesn't work with uncontrolled values, so we must use `onChange` instead and ignore enter key presses

+ convert EuiSelectableSearch from class to function component, because why not

kibanamachine · 2022-02-24T16:58:51Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5658/

cchaos

A bit of quick feedback.

cchaos · 2022-02-24T14:57:57Z

src/components/suggest/suggest.tsx

          onFocusBadge: false,
          paddingSize: 'none',
          isVirtualized,
-          ...rest,


👍 Makes much more sense. Can we also then add a listProps prop that can be passed down?

src/components/suggest/suggest.tsx

- that can be reused by other components repeating `searchProps` - while keeping internal `_` type for props passed only by EuiSelectable parent + update changelog to note removed `incremental` prop, but look into fixing `inputRef` prop

…ication

- search value isn't changing when options are clicked, so there's no need to call `searchProps.onChange`

…Props naming - except for onSearch which becomes transmogged to onChange - not sure if this is too confusing

…lectable functionality + this fixes the EuiSelectableTemplateSitewide demo to not error on Cmd+K keypress, + fixes Cmd+K keypress on Firefox going to the URL bar

kibanamachine · 2022-03-01T00:11:26Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5658/

kibanamachine · 2022-03-01T19:59:50Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5658/

kibanamachine · 2022-03-02T17:13:06Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5658/

thompsongl · 2022-03-02T22:46:14Z

src/components/suggest/suggest.tsx

  return (
    <>
      <EuiSelectable<EuiSuggestionProps>
-        id={id}


It's not relevant for a11y, but a consumer-provided id is no longer used throughout the component. That is, the listbox gets one id and the input gets another.
Previously, this id would be the basis for rootId and used in all child ids. rootId now always gets the fallback case.

So a consumer can't use the id for any testing/DOM operations (maybe that's not terrible?).

I made this change in EuiSuggest because it matched the component's behavior prior to the refactor (id and all other props were passed directly to the search input). For EuiSelectable itself I fixed this in 809ec87 - the wrapping div now receives the consumer id instead of the listbox receiving that id (I'm not quite sure why that change was made).

Yeah that makes sense. What I'm getting at is: why not use the consumer-provided id as the basis for all generated ids via rootId. As is, rootId no longer does much because props.id will always be undefined:

eui/src/components/selectable/selectable.tsx

Lines 188 to 190 in 50d3f02

this.rootId = props.id

? (suffix) => `${props.id}${suffix ? `_${suffix}` : ''}`

: htmlIdGenerator();

Ah gotcha. I can do that, I was kinda in "restore all the props to the search input" mode and wasn't sure if that constituted a breaking change. I guess we already changed it as of the refactor though so it makes sense to keep the id on the root EuiSelectable 👍

Actually sorry after looking at this again I don't think it's a 1:1 change especially from how EuiSuggest was previously. In the underlying EuiSelectable we don't actually automatically generate an id for the search input, just for the dropdown list and the message contents.

Until we add the option for listProps and searchProps (so users can manually pass in searchProps: { id: 'whatever' }) I'm really tempted to say we should leave the ID on the search input to maintain parity with how the component previously was.

I want to add that it also feels really weird that an <EuiSuggest id="foo" data-test-subj="bar" /> would have the id go on the parent wrapper but the data-test-subj go on the search input. For now/for consistency I think we should keep properties on the search input 🤷‍♀️

Until we add the option for listProps and searchProps

I'm fine waiting for this. Like I said, the id isn't relevant to a11y, just to potential consumer ease of knowing that id="foo" will result in <input id="foo" /> and other elements having predictable ids: foo_listbox, foo_instructions, foo_messageContent, etc.

kibanamachine · 2022-03-02T22:58:04Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5658/

thompsongl

LGTM

Don't forget about your [REVERT] commits

kibanamachine · 2022-03-03T16:48:07Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5658/

cee-chen · 2022-03-03T17:52:12Z

Latest merge was only a changelog conflict fix, so I'm going to go ahead and force-merge this without waiting for CI to finish, since the next release is waiting on this PR

kibanamachine · 2022-03-03T18:23:35Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5658/

cee-chen added the breaking change PRs with breaking changes. (Don't delete - used for automation) label Feb 23, 2022

cee-chen requested a review from thompsongl February 23, 2022 20:18

cee-chen force-pushed the suggest/5644 branch 2 times, most recently from f04927a to b2b0e90 Compare February 23, 2022 20:32

cee-chen commented Feb 23, 2022

View reviewed changes

cee-chen force-pushed the suggest/5644 branch from baf0eeb to d5ec22c Compare February 23, 2022 21:45

cee-chen added 10 commits February 24, 2022 08:26

Add playground to EuiSuggest component

3bc5cf6

- to make it easier to test missing props

[REVERT?] Add controlled value+onSearchChange console log to EuiSugge…

29e184e

…st example - to make it easier to test behavior - could also be left in to serve as a controlled example 🤷

[REVERT] Test controlled value EuiSelectable

d9c9f92

[EuiSuggest] Omit non-valid search props from types/documentation

9765ab6

Some of these were confusing to me when they showed up in the playground, some of these straight up don't work or break EuiSelectable behavior when passed, and some should be removed for API consistency

[EuiSuggest] Pass ...rest props to searchProps instead of listProps

53c328e

- to match previous behavior as well as handle EuiFieldSearch props

[EuiSuggest] Pass id prop to search input

10a2b57

[EuiSelectable] Render passed id on wrapping div + fix _listbox c…

809ec87

…hild ID - we should be passing the direct ID prop to the parent wrapper, not to the `ul` listbox, which previously had a `_listbox` suffix

[misc] Update snapshots, write more complete EuiSelectableSearch tests

aab8554

+ convert EuiSelectableSearch from class to function component, because why not

Add changelog entry

e883d8c

cee-chen force-pushed the suggest/5644 branch from d5ec22c to e883d8c Compare February 24, 2022 16:26

cchaos reviewed Feb 24, 2022

View reviewed changes

thompsongl mentioned this pull request Feb 28, 2022

[EuiSelectableSearch] Embedded <EuiFieldSearch> does not receive an onChange when <EuiSelectableSearch> is given searchProps.value #5651

Closed

cee-chen added 8 commits February 28, 2022 15:34

Fix onChange arg order to match what gets sent back to consuming appl…

2293fd2

…ication

[EuiSelectable] Remove searchProps.onSearch in favor of onChange

ee36eda

Remove unnecessary onChange callback on option click

b969621

- search value isn't changing when options are clicked, so there's no need to call `searchProps.onChange`

[EuiSuggest] Change callbacks to more closely match underlying search…

d37d0f4

…Props naming - except for onSearch which becomes transmogged to onChange - not sure if this is too confusing

Fix searchProps.inputRef to work as expected while not breaking EuiSe…

514b8f9

…lectable functionality + this fixes the EuiSelectableTemplateSitewide demo to not error on Cmd+K keypress, + fixes Cmd+K keypress on Firefox going to the URL bar

Merge branch 'main' into suggest/5644

50c6292

Merge branch 'main' into suggest/5644

92d70f3

Fix EuiSuggest Cypress tests to account for new callback names

3124bca

Merge branch 'main' into suggest/5644

9702d1f

Merge branch 'main' into suggest/5644

50d3f02

thompsongl reviewed Mar 2, 2022

View reviewed changes

thompsongl approved these changes Mar 3, 2022

View reviewed changes

Revert controlled value demos from docs examples

a75be5d

cee-chen enabled auto-merge (squash) March 3, 2022 16:14

Merge branch 'main' into suggest/5644

3c99a6a

cee-chen disabled auto-merge March 3, 2022 17:52

cee-chen merged commit a89ad8e into elastic:main Mar 3, 2022

cee-chen deleted the suggest/5644 branch March 3, 2022 17:52

thompsongl mentioned this pull request Mar 4, 2022

Upgrade EUI to v51.1.0 elastic/kibana#126926

Merged

	const suggestInput = (
	<EuiSuggestInput
	status={status}
	tooltipContent={tooltipContent}
	append={append}
	suggestions={suggestionList}
	{...rest}
	/>

	this.rootId = props.id
	? (suffix) => `${props.id}${suffix ? `_${suffix}` : ''}`
	: htmlIdGenerator();

Conversation

cee-chen commented Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

QA

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Feb 23, 2022

Uh oh!

kibanamachine commented Feb 23, 2022

Uh oh!

kibanamachine commented Feb 24, 2022

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Mar 1, 2022

Uh oh!

kibanamachine commented Mar 1, 2022

Uh oh!

kibanamachine commented Mar 2, 2022

Uh oh!

thompsongl Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Mar 2, 2022

Uh oh!

thompsongl left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Mar 3, 2022

Uh oh!

cee-chen commented Mar 3, 2022

Uh oh!

kibanamachine commented Mar 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cee-chen commented Feb 23, 2022 •

edited

Loading

thompsongl Mar 2, 2022 •

edited

Loading