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

Updated EuiComboBox to allow the options list to open for single selection custom options #3706

Merged

Conversation

miukimiu
Copy link
Contributor

@miukimiu miukimiu commented Jul 8, 2020

Summary

This PR fixes #3701.

Feature

Before this PR, when selecting a single selection custom option the options list wouldn't open. Consumers could think this was a bug. So we decided to improve the code and to always allow the list to open for single selection custom options.

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
  • [ ] Checked Code Sandbox works for the any docs examples
  • [ ] 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

@miukimiu miukimiu changed the title Fixing includes to return true when object exists in array Fix bug in EuiComboBox options list not opening when singleSelection and onCreateOption are true Jul 8, 2020
@kibanamachine
Copy link

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

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.

This change does not appear to fix the underlying issue in #3701. To test with, I modified the src-docs/src/views/combo_box/combo_box.js example by:

  • removing the options.push(newOption); action in onCreateOption
  • added singleSelection={{asPlainText: true}} to the combobox

A larger question though: do we really want to suppress the options list when the sole entry in a single selection is a custom option? As the original issue mentioned, it is still triggered when typing. Seems like this setup creates a less unified user experience.

@miukimiu
Copy link
Contributor Author

miukimiu commented Jul 14, 2020

@chandlerprall, not sure I understood the issue. 😛

I thought the problem was when we have singleSelection={{ asPlainText: true }} and onCreateOption and we click the input, the list was not showing up. Like it is demonstrated here: https://codesandbox.io/s/combobox-example-7bw9r?file=/index.js

This issue was fixed. You don't need to start typing to make the list showing up.

@kibanamachine
Copy link

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

@chandlerprall
Copy link
Contributor

There are two identified issues:

  1. [EuiComboBox] Options list doesn't show when singleSelection and onCreateOption are set #3701 where providing singleSelection and onCreateOption prevents the popover
  2. The one you identified in this PR description, where options and selectedOptions arrays do not share in-memory objects, but rather recreate them

Your PR addresses the second item, but not the first. To demonstrate, replace src-docs/src/views/combo_box/combo_box.js with the following

import React, { useState } from 'react';

import { EuiComboBox } from '../../../../src/components';
import { DisplayToggles } from '../form_controls/display_toggles';

const options = [
  {
    label: 'Titan',
    'data-test-subj': 'titanOption',
  },
  {
    label: 'Enceladus is disabled',
    disabled: true,
  },
  {
    label: 'Mimas',
  },
  {
    label: 'Dione',
  },
  {
    label: 'Iapetus',
  },
  {
    label: 'Phoebe',
  },
  {
    label: 'Rhea',
  },
  {
    label:
      "Pandora is one of Saturn's moons, named for a Titaness of Greek mythology",
  },
  {
    label: 'Tethys',
  },
  {
    label: 'Hyperion',
  },
];
export default () => {
  const [selectedOptions, setSelected] = useState([options[2]]);

  const onChange = selectedOptions => {
    setSelected(selectedOptions);
  };

  const onCreateOption = searchValue => {
    const normalizedSearchValue = searchValue.trim().toLowerCase();

    if (!normalizedSearchValue) {
      return;
    }

    const newOption = {
      label: searchValue,
    };

    // Select the option.
    setSelected([newOption]);
  };

  return (
    /* DisplayToggles wrapper for Docs only */
    <DisplayToggles canDisabled={false} canReadOnly={false}>
      <EuiComboBox
        singleSelection={{ asPlainText: true }}
        placeholder="Select or create options"
        options={options}
        selectedOptions={selectedOptions}
        onChange={onChange}
        onCreateOption={onCreateOption}
        isClearable={true}
        data-test-subj="demoComboBox"
      />
    </DisplayToggles>
  );
};

Then in that first Combo box example, add a custom entry. The popover will now not trigger by clicking on the input.

I took an additional step back in the response by asking: do we really want to prevent that popover in any case? The component is structured in a way where the issue in #3701 is a feature, not a bug. If we're changing that decision, we should be able to remove the call to isSingleSelectionCustomOption from onComboBoxFocus.

onComboBoxFocus: FocusEventHandler<HTMLInputElement> = event => {
if (this.props.onFocus) {
this.props.onFocus(event);
}
if (!this.isSingleSelectionCustomOption()) {
this.openList();
}
this.setState({ hasFocus: true });
};

@miukimiu
Copy link
Contributor Author

Thanks @Chandler for helping me out!

I think it makes more sense to always allow the list to open for single selection custom options. So I worked on adding this new feature.

I also provided a new example in our docs.

Screenshot 2020-07-15 at 12 32 16

@miukimiu miukimiu changed the title Fix bug in EuiComboBox options list not opening when singleSelection and onCreateOption are true Updated EuiComboBox to allow the options list to open for single selection custom options Jul 15, 2020
@kibanamachine
Copy link

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

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.

Couple more comments

src/components/combo_box/combo_box.tsx Show resolved Hide resolved
@kibanamachine
Copy link

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

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

I think this is a good place to also showcase a best-practice of supplying the user with help text indicating the ability to create a custom option. For instance, we do this in Visualize where the combo box is used for a time interval that has a few defaults, but a user can also type in their own.

Screen Shot 2020-07-16 at 12 01 38 PM

Can you add some text in the example about providing help text when allowing for custom options and also wrap the example in a EuiFormRow with an example help text?

@kibanamachine
Copy link

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

1 similar comment
@kibanamachine
Copy link

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

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.

Code & doc changes LGTM! Left one small comment on the Single selection example, but I don't want to be a blocker as I'm out tomorrow.

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.

Extra stamp of approval. Thanks for all the iterations on this one!

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@miukimiu
Copy link
Contributor Author

Thanks @chandlerprall.

@cchaos I improved the example. Can you take another look?

@miukimiu miukimiu requested a review from cchaos July 17, 2020 12:27
@miukimiu
Copy link
Contributor Author

jenkins test this

@kibanamachine
Copy link

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

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

LGTM! Works as described. Just a couple nit-picky items.

src-docs/src/views/combo_box/combo_box_example.js Outdated Show resolved Hide resolved
src-docs/src/views/combo_box/combo_box_example.js Outdated Show resolved Hide resolved
src-docs/src/views/combo_box/single_selection.js Outdated Show resolved Hide resolved
miukimiu and others added 5 commits July 17, 2020 16:33
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@kibanamachine
Copy link

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

@miukimiu miukimiu merged commit 518419c into elastic:master Jul 17, 2020
anishagg17 added a commit to anishagg17/eui that referenced this pull request Jul 20, 2020
* Break up release.js (elastic#3723)

* Switch release.js to use arguments instead of env vars

* Switch MFA code back to env var so it doesn't leak in CI logs

* Update job definition to use --type arg

* Support breaking up release steps with args

* Break release up to fetch time-sensitive MFA token right before publish

* Strip whitespace from each step

Co-authored-by: Chandler Prall <chandler.prall@gmail.com>

Co-authored-by: Chandler Prall <chandler.prall@gmail.com>

* Allow prop-setting on the FlexItems within DescribedFormGroup (elastic#3717)

* Allow prop-setting on the FlexItems within DescribedFormGroup

* Add changelog entry

* Honor classes on fieldFlexItem

* Update src/components/form/described_form_group/described_form_group.test.tsx

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

* Update snap name

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

* Re-focus EuiSuperSelect input after making a value change (elastic#3734)

* Re-focus EuiSuperSelect input after making a value change

* changelog

* [EuiStat] Allow customizing the render of the title and description HTML tags (elastic#3693)

* Add titleElement and descriptionElement to EuiStat

* Updated snapshots

* Updated changelog

* Fixed issue with screenreader

* Updated snapshots

* Use screen reader only if title and description is a string

* updated snapshots

* Merge remote-tracking branch 'upstream/master' into fix/stat

* Fixed typo in changelod

* removed titleChildren

* titlechildren as variable

* Update CHANGELOG.md

Remove an extra line left over from a merge resolution

Co-authored-by: Chandler Prall <chandler.prall@gmail.com>

* [EuiTable] Expand item action name to allow a function (elastic#3739)

* allow for item -> ReactNode in name

* docs

* CL

* Add ssh keys so new tags can be pushed to Github from Jenkins (elastic#3740)

* Add ssh keys so new tags can be pushed to Github

* Need a vault token before we can pull secrets

* update i18ntokens

* 27.1.0

* Updated documentation.

* Chore/decouple button content (elastic#3730)

* [New] EuiButtonContent

For rendering the contents of buttons, icon (loading spinner) and text wrapper for children

* Use EuiButtonContent in EuiButtonEmpty

* Fixing classNames and disabled states

* Create EuiButtonDisplay for internal usage

* Fix snaps

* ts gymnastics

* Added tests for EuiButtonContent

* More optimization

- Extend EuiButtonContentProps
- Content styles are in button_content.scss

* Restricted list of `element`s

* [Docs] Adding more acccessibility-focused notes and examples (elastic#3714)

* making more a11y callouts in docs

* Apply suggestions from code review

Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com>

* prettier update

Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com>

* [EuiFocusTrap] Use `react-focus-on` (elastic#3631)

* WIP: react-focus-on

* WIP: fix GuideFullScreen

* noIsolation; onClickOutside

* euiflyout snapshot updates

* euiflyout snapshot updates

* euimodal snapshot updates

* euidatagrid snapshot updates

* euicolorpalettepicker snapshot updates

* euisuperselect snapshot updates

* euicollapsible snapshot updates

* euifocustrap snapshot updates

* allow for array snapshots with takeMountedSnapshot

* docs improvements

* default to noIsolation=true and scrollLock=false

* CL

* Fixed bug in EuiComboBox always showing a scrollbar (elastic#3744)

* Fixed EuiComboBox always showing a scrollbar

* Adding the right PR number to CL

* Added useEuiI18n hook (elastic#3749)

* Added useEuiI18n hook

* Updated docs with useEuiI18n hook, added snippets

* Add support to fetch-1i8n-strings or useEuiI18n to match EuiI18n extraction

* Fix up return types for useEuiI18n

* Updated custom eslint i18n rule/package to lint useEuiI18n usages

* changelog

* Remove something I was testing with and lost where I had placeed it.

* [EuiSuperDatePicker] Bypass formatting `null` dates (elastic#3750)

* prevent formatting on null value

* remove unnecessary cast

* account for keyboard nav with null selection

* CL

* Updated EuiComboBox to allow the options list to open for single selection custom options (elastic#3706)

* Fixing includes to return true when object exists in array

* changelog

* Allowing list to open for single selection custom options

* Updated changelog

* PR review

* Improving example

* Improving example

* Addind isClearable

* Improving examples

* Improving explanation text

* Adding note

* Addressing PR issues

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

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

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

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

* Snippet

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

* Add analyze event glyph to EuiIcon (elastic#3729)

* adding analyze event security icon

* updating analyze_event icon

* updating again

* Update CHANGELOG.md

* Update consuming.md (elastic#3769)

* Path has changed and the wiki has not been updated.

* Fix zIndex for two popup ups (elastic#3768)

Clicking both buttons on https://eui.elastic.co/#/tabular-content/data-grid-styling-and-toolbar demo brings up partially hidden popups because their z-index is too low. Increasing by one seems to do the trick.

* [Playground] EuiBadge,   EuiNotificationBadge,   EuiBetaBadge (elastic#3722)

* created playground for badges

* removed commented code

* used validator for iconType and colour

* updated variable name

* removed colour validation

* removed unnecessary imports

* removed default values, fullscreen mode

* suggesstions

* removed placeholder, added required, some badge props set to string

* used actual value of child in text field

* added sanitize function

* fixed unique-key warning

* added validation

* updated to identify the change whenthe state changes

* suggestions

* added onCLick function snippet

* removed error popup by react-view

* removed lint err

* removed commented code

Co-authored-by: Josh Mock <joshua.mock@elastic.co>
Co-authored-by: Chandler Prall <chandler.prall@gmail.com>
Co-authored-by: Scott Gould <sbg@elastic.co>
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-authored-by: Ashik Meerankutty <ashik9591@gmail.com>
Co-authored-by: Greg Thompson <thompson.glowe@gmail.com>
Co-authored-by: Michail Yasonik <michail.yasonik@elastic.co>
Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com>
Co-authored-by: Elizabet Oliveira <elizabet.oliveira@gmail.com>
Co-authored-by: Marra Sherrier <45169477+marrasherrier@users.noreply.github.com>
Co-authored-by: Alberto Andújar <josealbertoandujar@gmail.com>
Co-authored-by: Yuri Astrakhan <yuriastrakhan@gmail.com>
anishagg17 pushed a commit to anishagg17/eui that referenced this pull request Jul 20, 2020
…ction custom options (elastic#3706)

* Fixing includes to return true when object exists in array

* changelog

* Allowing list to open for single selection custom options

* Updated changelog

* PR review

* Improving example

* Improving example

* Addind isClearable

* Improving examples

* Improving explanation text

* Adding note

* Addressing PR issues

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

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

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

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

* Snippet

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
anishagg17 added a commit to anishagg17/eui that referenced this pull request Jul 20, 2020
anishagg17 added a commit to anishagg17/eui that referenced this pull request Jul 20, 2020
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] Options list doesn't show when singleSelection and onCreateOption are set
4 participants