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

[EuiComboBox] Fix delimited search value option creation #3841

Merged
merged 6 commits into from
Aug 11, 2020

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Jul 31, 2020

Summary

Fixes #3794, where a copy-pasted delimited search value would only add the last item in the delimited list.

The problem stemmed from looped addCustomOption calls not updating props.selectedOptions fast enough for the next addCustomOption call to have access to the updated list, effectively resetting the selected list each time until the last.

Because addCustomOption is provided by the consumer, we can't control its logic. This proposes a simple no-delay setTimeout, which gives enough time to finish the execution queue between calls.

Updated the docs example to more appropriately update using the useState callback parameter.

Other docs change is just an update for best practices: keep data in state so it's more visible to React.

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in IE11 and Firefox
- [ ] Props have proper autodocs
- [ ] Added documentation

- [ ] Added or updated jest tests

  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

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

@chandlerprall
Copy link
Contributor

I wonder if a better option is to update our examples' calls of setSelected to allow chaining those updates,

// Select the option.
setSelected(previousSelection => [...previousSelection, newOption]);

Especially in the React async rendering future, I'm wary of using a delay to fix a state update, when that update is tied to waiting for React to re-render in time.

@thompsongl
Copy link
Contributor Author

I went back and forth on whether to designate this a documentation issue or not. The async rendering future argument has me reconsidering my choice.

I'll try it out

@kibanamachine
Copy link

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

Copy link
Contributor

@miukimiu miukimiu left a comment

Choose a reason for hiding this comment

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

I tested passing a,b,c and hitting enter and it works. 🎉

The only thing that I noticed is that the empty state that it's showing is the one for the custom options:

Screenshot 2020-08-03 at 14 44 29

There is an empty state for the delimiter on combo_box_options_list.tsx:

emptyStateContent = (
<div className="euiComboBoxOption__contentWrapper">
    <p className="euiComboBoxOption__emptyStateText">
        <EuiI18n
            token="euiComboBoxOptionsList.delimiterMessage"
            default="Add each item separated by {delimiter}"
            values={{ delimiter: <strong>{delimiter}</strong> }}
        />
    </p>
    {hitEnterBadge}
</div>
);

@kibanamachine
Copy link

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

@chandlerprall
Copy link
Contributor

flaky test jenkins test this

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Let's make the options.push(newOption); -> setOptions([...options, newOption]); change to all custom-entry examples to help avoid this coming up again in a future variation. Otherwise this LGTM

@kibanamachine
Copy link

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

Copy link
Contributor

@miukimiu miukimiu left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes. I tested again and it looks good! 🎉

@thompsongl thompsongl merged commit 6b9d34b into elastic:master Aug 11, 2020
nyurik pushed a commit that referenced this pull request Aug 18, 2020
* timeout between addCustomOption calls

* fix the docs example

* delimeter empty state prompt

* CL

* keep changeable options in state
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.

[EuiComboBox] Delimiter functionality not working when pasting list
4 participants