[EuiComboBox] Allow options to have a unique key by timroes · Pull Request #4048 · elastic/eui

timroes · 2020-09-17T10:57:22Z

Summary

This will allow EuiComboBoxItems to optionally have an id and in this case allow duplicate labels. We require this change for Kibana, to allow display displayName of fields instead of the real name, which in some cases could be duplicate.

I added a documentation section, that also states that this is not recommended to use duplicate labels.

@chandlerprall It would be great if you could check if I've forgot any place to make this work. It seems to work exactly as I want, and I tried to go through all combo box code, but a second pair of eyes would be really appreciated.

Checklist

~~[ ] Check against all themes for compatibility in both light and dark modes~~
[ ] Checked in mobile
[ ] Checked in Chrome, Safari, Edge, 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

kibanamachine · 2020-09-17T11:18:24Z

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

kibanamachine · 2020-09-17T14:09:36Z

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

kibanamachine · 2020-09-17T14:37:37Z

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

kibanamachine · 2020-09-21T15:00:20Z

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

chandlerprall

The approach looks good to me, and I was unable to break it. Added a couple small requests/thoughts.

CHANGELOG.md

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

src/components/combo_box/matching_options.ts

kibanamachine · 2020-09-22T08:32:48Z

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

chandlerprall

Changes LGTM; pulled & tested existing examples & the new one locally

cchaos

Thanks the PR and the documentation example! Gots a couple of comments.

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

cchaos · 2020-09-22T21:42:44Z

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

      <EuiSpacer />

-      <EuiCallOut title="No duplicate option labels allowed" color="warning">
+      <EuiCallOut title="Duplicate labels require an id" color="warning">


Let's actually remove this entire callout now that we have a specific example section that explains this more succinctly.

cchaos · 2020-09-22T21:44:30Z

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

+  };
+
+  return (
+    /* DisplayToggles wrapper for Docs only */


Remove this comment as it doesn't apply here.

Suggested change

/* DisplayToggles wrapper for Docs only */

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

+import DuplicateOptions from './combo_box_duplicates';
+const duplicateOptionsSource = require('!!raw-loader!./combo_box_duplicates');
+const duplicateOptionsHtml = renderToHtml(DuplicateOptions);
+


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

+        </p>
+      ),
+      props: { EuiComboBox },
+      demo: <DuplicateOptions />,


src/components/combo_box/combo_box_input/combo_box_input.tsx

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

kibanamachine · 2020-09-28T09:35:37Z

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

kibanamachine · 2020-09-28T09:58:03Z

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

chandlerprall · 2020-09-30T16:59:54Z

Talked with Tim, it is likely that #4072 introduces a new location this change around key vs. label will need to address, so he is holding off until that merges. The downstream source for any urgency here has shifted and is no longer an immediately pressing issue, so this delay is fine.

…ombo-box-ids

kibanamachine · 2020-10-06T14:02:39Z

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

cchaos

Changing id to key solves my concerns. Thanks!

chandlerprall

Changes look even better now :) Pulled & tested locally.

timroes · 2020-10-06T15:00:55Z

Jenkins test this (unrelated CI issue)

kibanamachine · 2020-10-06T15:23:21Z

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

* Allow options in combo box to have an id * Change warning in documentation * Fix eslint error * Added changelog entry * Address review * Update src-docs/src/views/combo_box/combo_box_example.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Address review feedback * Rename id to key * Address recently merged PRs changes Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

Allow options in combo box to have an id

edd5d4e

timroes requested a review from chandlerprall September 17, 2020 10:57

timroes added 2 commits September 17, 2020 13:20

Change warning in documentation

71636b6

Fix eslint error

02f2de7

Added changelog entry

48d0d63

Merge branch 'master' into fix/combo-box-ids

af26b6f

chandlerprall suggested changes Sep 21, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

src-docs/src/views/combo_box/combo_box_example.js Outdated Show resolved Hide resolved

src/components/combo_box/matching_options.ts Show resolved Hide resolved

Address review

094b5bf

chandlerprall approved these changes Sep 22, 2020

View reviewed changes

cchaos suggested changes Sep 22, 2020

View reviewed changes

timroes and others added 2 commits September 28, 2020 11:13

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

0d1801c

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

Address review feedback

af0d0a8

kertal mentioned this pull request Sep 28, 2020

[Discover] Allow custom label for fields via index pattern field management elastic/kibana#70039

Merged

6 tasks

timroes added 4 commits October 2, 2020 15:52

Merge branch 'master' into fix/combo-box-ids

070aeca

Rename id to key

33041b6

Merge branch 'master' into fix/combo-box-ids

d7e8665

Merge branch 'fix/combo-box-ids' of github.com:timroes/eui into fix/c…

4f208bc

…ombo-box-ids

cchaos approved these changes Oct 6, 2020

View reviewed changes

cchaos changed the title ~~Allow options in combo box to have an id~~ Allow options in combo box to have a unique key Oct 6, 2020

Address recently merged PRs changes

c866a0b

cchaos changed the title ~~Allow options in combo box to have a unique key~~ [EuiComboBox] Allow options to have a unique key Oct 6, 2020

chandlerprall approved these changes Oct 6, 2020

View reviewed changes

thompsongl merged commit 2f83517 into elastic:master Oct 6, 2020

timroes deleted the fix/combo-box-ids branch October 6, 2020 15:44

chandlerprall mentioned this pull request Oct 15, 2020

Upgrade EUI to v29.5.0 elastic/kibana#80753

Merged

Conversation

timroes commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

kibanamachine commented Sep 17, 2020

Uh oh!

kibanamachine commented Sep 17, 2020

Uh oh!

kibanamachine commented Sep 17, 2020

Uh oh!

kibanamachine commented Sep 21, 2020

Uh oh!

chandlerprall left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Sep 22, 2020

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cchaos Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

cchaos Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

kibanamachine commented Sep 28, 2020

Uh oh!

kibanamachine commented Sep 28, 2020

Uh oh!

chandlerprall commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kibanamachine commented Oct 6, 2020

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

Uh oh!

timroes commented Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kibanamachine commented Oct 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

timroes commented Sep 17, 2020 •

edited

Loading

chandlerprall left a comment •

edited

Loading

chandlerprall commented Sep 30, 2020 •

edited

Loading

timroes commented Oct 6, 2020 •

edited

Loading