refactor(selectable): avoid prop conflicts #4291

smhutch · 2020-11-20T13:15:09Z

Summary

I'm planning to open another PR to address #4262. While looking starting working on that, I noticed some code patterns that I see as problematic. The issue I see is that we have cases of props conflicting with each other (in this case top-level props conflicting with seachProps). The current code mostly fixes these issues by creating a ...cleanedSearchProps, which is all of searchProps minus some de-structured values. While this approach works, it's quite unstable. Another engineer could easily confuse these unused variables as "useless", and remove them. Similarly, we need to jump through some hoops to keep TypeScript happy about this approach (in this type casting).

The approach I have taken instead is to not spread ...searchProps over all others. Instead, I have continued to spread these, but added a couple of explicit overrides afterwards.

As this change doesn't explicitly fix an open issue, and is to some extent an opinionated change. I'm interested in hearing some thoughts on my approach from the Elastic team. Thanks!

Related to

#4132
#4262

Checklist

kibanamachine · 2020-11-20T13:15:11Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

smhutch · 2020-11-20T13:17:05Z

src/components/selectable/selectable.tsx

-    ) => {
-      if (props && props['aria-label']) {
-        return { 'aria-label': props['aria-label'] };
+    const getAriaProps = (


I renamed this function, because it's now setting multiple props, or returning undefined if no suitable aria props can be set.

smhutch · 2020-11-20T13:17:38Z

src/components/selectable/selectable.tsx

+    type AriaLabelProps = {
+      'aria-label': string;
+      'aria-describedby': undefined;
+    };


When using aria-label, always override aria-describedby.

smhutch · 2020-11-20T13:17:56Z

src/components/selectable/selectable.tsx

+    type AriaDescribedbyProps = {
+      'aria-label': undefined;
+      'aria-describedby': string;
+    };


As above, do not set aria-label, if aria-described is used.

I think you changed this logic a little (which was, granted, somewhat convoluted)... Either way, the intended effect is that both aria-label and aria-describedby is allowed. We just need to move where the attribute is being set.

@myasonik I'm still not sure where the logic has changed (but I agree, it's somewhat convoluted, and I could certainly be missing something).

I'm happy to make some more changes to accommodate the expected behaviour. Based on your last comment, I think the required logic is:

aria-label

Uses searchProps['aria-label'] or listProps['aria-label'] if set

Else, uses aria-label passed to EuiSelectable

Uses a fallback of placeholderName

aria-describedby

Uses searchProps['aria-describedby'] or listProps['aria-describedby'] if set

Else, uses aria-describedby passed to EuiSelectable

In either of the above cases, messageContentId is added to the end.

No fallback is used.

Does this look correct? Thanks!

Sorry about the long delay on getting back to you @smhutch - I was off for US Thanksgiving for a while but am back at it now!

I think what you outlined is generally correct but I'm going to restate it in a little more depth just to make sure we're on the same page.

First, let's define where we're going to be setting these props. There are 2 possible spots:
1. the input field (which has these values passed along through the ...rest param)
2. the list box (which sets these values using some ref hackiness)

The overarching rule is that each of these can be controlled individually with searchProps or listProps respectively but also need to respect broader values set on EuiSelectable itself.

So, for the input field:

aria-label

Use searchProps['aria-label']

Use aria-label passed into EuiSelectable

Use placeholderName

aria-describedby

Use searchProps['aria-describedby'] and messageContentId as a space-separated string

Use aria-label passed into EuiSelectable and messageContentId as a space-separated string

Use messageContentId

For the list box, it is the same as above but replace searchProps with listProps.

Writing this up, I also realized we have a gap by not covering aria-labelledby in conjunction with aria-label but I think we ignore that for now and tackle it in a future PR. (If you do want to tackle it, let me know and I can write up some guidelines but it'll double the length of the aria-label logic...)

smhutch · 2020-11-20T13:19:23Z

src/components/selectable/selectable.tsx

+    type AriaProps = Partial<{
+      'aria-label': string;
+      'aria-describedby': string;
+    }>;


Previously this was:

| Partial<EuiSelectableSearchProps<T>> | EuiSelectableOptionsListPropsWithDefaults

I changed to only require the keys that are needed within this function. Any object that might have these set may be used with this function.

smhutch · 2020-11-20T13:20:06Z

src/components/selectable/selectable.tsx

+    const getAriaProps = (
+      props: AriaProps,
+      describedbyId?: string
+    ): AriaPropsReturn => {


Added an explicit return type. Not 100% needed, but I think it's helpful.

smhutch · 2020-11-20T13:22:22Z

src/components/selectable/selectable.tsx

-              : { 'aria-label': placeholderName })}
-            {...cleanedSearchProps}
+            {...searchProps}
+            onChange={this.onSearchChange}


Instead of spreading ...cleanedSeachProps, I took the approach of not spreading at the end. Any values that come after ...searchProps will override those which are spread.

In this case, onChange and ...ariaProps will override whatever is passed to ...searchProps

I have a feeling that there are more potential issues lurking here (i.e. should searchProps.placeholder override placeholder?)

smhutch · 2020-11-20T13:23:17Z

src/components/selectable/selectable.tsx

      singleSelection,
      isLoading,
-      listProps,
+      listProps = {},


Setting this to an empty object by default, avoids the possibility of it being undefined. Which simplifies some other code (no need to check that it's not undefined, before accessing properties). 😄

smhutch · 2020-11-20T13:24:33Z

src/components/selectable/selectable.tsx

@@ -333,9 +333,6 @@ export class EuiSelectable<T = {}> extends Component<
        }
      }
    );
-    if (this.props.searchProps && this.props.searchProps.onSearch) {
-      this.props.searchProps.onSearch(searchValue);
-    }


Here we were calling searchProps.onSearch, from a handler which was passed down as onChange. This code is still called, but is now handled within selectable_search.tsx

smhutch · 2020-11-20T13:26:17Z

src/components/selectable/selectable_search/selectable_search.tsx

+
+    if (this.props.onSearch) {
+      this.props.onSearch(value);
+    }


Allows consumers to hook into onSearch calls.

smhutch · 2020-11-20T13:26:45Z

src/components/selectable/selectable_search/selectable_search.tsx

        incremental
        defaultValue={defaultValue}
        fullWidth
        autoComplete="off"
        aria-haspopup="listbox"
        {...ariaPropsIfListIsPresent}
        {...rest}
+        onSearch={this.onSearchChange}


onSearch is moved to here, to prevent ...rest from overriding it.

thompsongl · 2020-11-20T15:50:26Z

Thanks for the PR, @smhutch!

Pinging @myasonik to check out the ARIA refactor

smhutch · 2021-01-18T17:20:29Z

@thompsongl @myasonik

Just revisiting this. I won't have time to update this PR for at least a few more weeks, but I do intend to come back and make the required changes.

thompsongl · 2021-01-19T17:45:59Z

👍 Thanks for the update, @smhutch

github-actions · 2021-04-20T00:03:50Z

👋 Hey there. This PR hasn't had any activity for 90 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

smhutch · 2021-04-20T07:15:25Z

Hey @thompsongl, I started a new job, and don't have time to resolve this any more. Closing for now.

refactor(selectable): avoid prop conflicts

94860ff

smhutch commented Nov 20, 2020

View reviewed changes

smhutch marked this pull request as ready for review November 20, 2020 13:35

smhutch mentioned this pull request Nov 20, 2020

[EuiSelectable] searchProps.onChange returns stale state #4262

Closed

thompsongl self-requested a review November 20, 2020 15:45

github-actions bot added the stale-pr label Apr 20, 2021

smhutch closed this Apr 20, 2021

smhutch deleted the selectable-cleanup branch April 20, 2021 07:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(selectable): avoid prop conflicts #4291

refactor(selectable): avoid prop conflicts #4291

smhutch commented Nov 20, 2020 •

edited

Loading

kibanamachine commented Nov 20, 2020

smhutch Nov 20, 2020

smhutch Nov 20, 2020

smhutch Nov 20, 2020

myasonik Nov 20, 2020

smhutch Nov 24, 2020 •

edited

Loading

myasonik Dec 2, 2020

smhutch Nov 20, 2020

smhutch Nov 20, 2020

smhutch Nov 20, 2020

smhutch Nov 20, 2020

smhutch Nov 20, 2020

smhutch Nov 20, 2020

smhutch Nov 20, 2020

thompsongl commented Nov 20, 2020

smhutch commented Jan 18, 2021

thompsongl commented Jan 19, 2021

github-actions bot commented Apr 20, 2021

smhutch commented Apr 20, 2021

refactor(selectable): avoid prop conflicts #4291

refactor(selectable): avoid prop conflicts #4291

Conversation

smhutch commented Nov 20, 2020 • edited Loading

Summary

Related to

Checklist

kibanamachine commented Nov 20, 2020

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

smhutch Nov 24, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thompsongl commented Nov 20, 2020

smhutch commented Jan 18, 2021

thompsongl commented Jan 19, 2021

github-actions bot commented Apr 20, 2021

smhutch commented Apr 20, 2021

smhutch commented Nov 20, 2020 •

edited

Loading

smhutch Nov 24, 2020 •

edited

Loading