Add icon to combo box #133

BSFishy · 2022-11-22T18:17:37Z

Description

Add a search icon prop to the combo box

Issues Resolved

#130

Check List

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Matt Provost <[email protected]>

ashwin-pc · 2022-11-23T02:34:43Z

I've called this out in the parent issue too, but i think we should not limit this to just the search icon, but rather any Oui Icon. even custom ones.

KrooshalUX · 2022-11-23T04:01:20Z

@ashwin-pc can you provide a "why" on not limiting the icon? For an icon in the left placement in a search bar, I would find it confusing to be any other icon. Flexibility on the right and on input boxes I am a OK with , but since the function of this field is search, I am providing this guardrail to prevent a misleading user experience from being implemented (an icon on the left indicates what the purpose of the field is, which is to search) Would like to hear your perspective though!

ashwin-pc · 2022-11-23T04:29:45Z

Its just my opinion, but i want the design system to offer me as much flexibility as possible while letting me do the right thing easily. By changing the property to simply icon, It lets me do both. When I want to indicate search, I can use icon="search", but is i want a combobox that indicates a list of tags that I am selecting, id prefer the icon="tag" option since thats more appropriate there. This lets me add the search icon when needed without making the flag seem like an afterthought having no other purpose than to just add a single icon to the input.

If this change was perhaps tied to a search feature in the combobox, i'd be more okay with the limitation since adding a prop like search=true indicates that this combobox will now act as a multi select search input and not just show an icon.

KrooshalUX · 2022-11-27T21:15:04Z

@ashwin-pc - These are some good points about implementation. I did finally think of one case where I would agree the search icon would not be the appropriate choice - when a combobox is used to create custom tags and there are no pre-existing tags. Search in this case is invalid, and perhaps "plus" is more appropriate. I know this is getting into splitting some hairs.

After some re-thinking and discussion, here are my updated thoughts to the approach of having the icon prop. I want engineering maintainers within the project to set the path for implementation and developer experience - so these are for consideration given the feedback in the comments.

Approach 1:

Add the icon prop type as configurable but to search by default
Allow icon display to true/false
This approach gives UX the opportunity to set a "common sense default" without having to be heavy handed in implementation approach. It does add some friction but preserves the freedom of choice

Approach 2:

Keep as-is and mark the changes with our experimental tag in order to measure impact on developer experience and UX. There is no reason that OUI cannot have the same approach to component changes as the OpenSearch application has. EUI even had a "beta" for their component changes as well, signaling that some prop definitions were not yet set in stone.

ashwin-pc · 2022-11-29T01:37:05Z

I like both approaches but i'd lean towards option 1.1 where we add an icon prop. But I dont think adding search by default is a good idea. It would mean that to disable an icon in a combobox, we would have to explicitly pass icon={false} to not show one. Not very fond of this as a pattern.

One other option would be to use the prop to define the purpose of the UI element. e.g. <OuiComboBox as="search" /> This styles the component as a search only component that cannot add new values dynamically. With this prop on, a search icon being always present makes sense. And if we want to add additional flexibility, we can add an icon prop too that lets the user override this icon or disable it all together.

KrooshalUX · 2022-11-29T01:41:48Z

For clarity & my education (thanks for taking me on the journey) - we wouldn’t want two separate props 1) icon type and 2) show/hide icon Vs icon = false? @BSFishy any approach ideas here as well?

kgcreative · 2022-11-29T16:46:23Z

I would be supportive of:

Keeping the default API the same
Adding an icon property
For ease of use, icon, icon=true and icon="search" should be equivalent <- (I think this is what @KrooshalUX meant when she said "search" as default -- not search icon on by default on the component, but rather, when the icon prop is used, we should show the search icon if no other information is passed)
icon="false", or not even passing the icon prop should result in the component not rendering it.

ashwin-pc · 2022-11-30T02:00:04Z

@kgcreative I can get behind that. Still not 100% onboard with the need to have icon=true default to search, but it does not take away from whats expected of the icon property.

kgcreative · 2022-11-30T03:36:58Z

I think one conversation to have is which icon makes the most sense to be "default" icon. When I look at the list of glyphs, the ones that come to mind are tag, dot, email, apps, home, and search, and looking at the usage of combobox in the application, search seems to be the most common case. I would rather icon=true not return an error, so more than anything this is graceful degradation

KrooshalUX · 2022-11-30T05:26:53Z

@kgcreative can you provide cases for setting those other options as default - my main justification for setting the default to search it is the primary interaction within the input box - search to find a tag (object,data source,index, etc) or to search & receive no result in the case where custom tag is added. To match the current default of the input box and to not force a braking visual change the icon would not be visible by default. This allows UX & builders to determine the “correct” approach across various projects.

kgcreative · 2022-11-30T18:27:03Z

@KrooshalUX, I think we have a clear use case for search. @ashwin-pc , I would be curious to know if you're aware of any other use cases that would weigh us toward using a different icon vs search as the default prop for icon. My sense was dot or apps could serve as a "generic" placeholder, but that would force developers to always pass a prop with icon even if their use case was the same. This is more of a developer ergonomics + "make it easy to do the right thing by default" issue.

AMoo-Miki · 2022-12-03T02:30:36Z

I am having trouble imagining a form that has 5 combo-boxes, all with a search icon to their left.
I wouldn't mind if the icon appeared only when active but doesn't an icon inside the field denote that that field is to search for something? Am i too old?

kgcreative · 2022-12-03T18:05:58Z

I am having trouble imagining a form that has 5 combo-boxes, all with a search icon to their left.

I wouldn't mind if the icon appeared only when active but doesn't an icon inside the field denote that that field is to search for something? Am i too old?

The intent is that a combo box with an icon be more an exception that the rule, but when adding an icon, often time the search icon is the most appropriate (for example for searching/ multi selecting an index, a field, or a tag)

ashwin-pc · 2022-12-09T09:08:02Z

@kgcreative @KrooshalUX The primary concern i have with this is the API. having a boolean prop that simply shows a search icon is indicative of code smell. If as @AMoo-Miki said, the icon showed up during user input or when it active automatically, thats fine. Or if the prop allowed for arbitrary icons too, it seems like a good use of a prop, but simply using it to show or hide a single icon "search" seems wasteful and indicative that we havent abstracted the problem well.

If we are doing this to not introduce a breaking change, thats something we should avoid since technically we can always find a solution does not break semver if we bend over backwards enough. But we should weigh that with the consequence of doing so. In this case its an API that serves a very specific usecase that imo can be solved without it.

API changes are hard to walk back on so we should be careful when introducing them.

KrooshalUX · 2022-12-09T19:32:00Z

@ashwin-pc I believe we have identified a path forward that satisfies your concerns. @BSFishy can you please follow up with the approach you are going to move forward with? Do we need to close this PR?

BSFishy · 2022-12-09T19:39:14Z

No, I'll update this one. I think the way forward is making an icon prop (instead of searchIcon), allowing it to be an icon string, boolean (true for search icon, false for no icon), or undefined (indicating no icon). Is this an agreeable solution @KrooshalUX @ashwin-pc ?

ashwin-pc · 2022-12-09T20:11:59Z

yep that works for me :)

Signed-off-by: Matt Provost <[email protected]>

ashwin-pc

The rest of the PR looks good, just update the test to explicitly check for the Icon element

src/components/combo_box/combo_box.test.tsx

Signed-off-by: Matt Provost <[email protected]>

joshuarrrr

This implementation looks good, but I think the props of the combo box are now confusing with two icon-related props that control different icons.

joshuarrrr · 2023-01-06T19:00:23Z

src/components/combo_box/combo_box_input/combo_box_input.tsx

@@ -59,6 +60,7 @@ export interface OuiComboBoxInputProps<T> extends CommonProps {
  focusedOptionId?: string;
  fullWidth?: boolean;
  hasSelectedOptions: boolean;
+  icon?: IconType | boolean;


I don't love that there's now both icon and noIcon props - it's confusing to reason about what should happen if those are in disagreement. Without reading through the source code it, would be confusing to know what would happen if ...icon="true" noIcon="true".... I understand if this change is meant to be backwards compatible. But for a PR that targets main, I'd expect us to deprecate noIcon in favor of icon.

joshuarrrr · 2023-01-06T19:12:43Z

src/components/combo_box/combo_box_input/combo_box_input.tsx

+    let icon;
+    if (!!iconProp) {
+      const iconClasses = classNames('ouiComboBoxPill', 'ouiComboBoxIcon');
+
+      icon = (
+        <OuiIcon
+          type={typeof iconProp === 'string' ? iconProp : 'search'}
+          size={compressed ? 's' : 'm'}
+          className={iconClasses}
+          data-test-subj="comboBoxIcon"
+        />
+      );
+    }


nit - you can avoid using let by rewriting as a function.

joshuarrrr · 2023-01-06T19:15:02Z

src/components/combo_box/combo_box_input/combo_box_input.tsx

    if (!noIcon) {
-      icon = {
+      formControlIcon = {


This change now makes it clear that noIcon and icon properties refer to different icons. Updating the name of noIcon to noFormControlIcon or noToggleIcon might be more descriptive.

* Add search icon to combo box Signed-off-by: Matt Provost <[email protected]> * Change from search icon to generic icon Signed-off-by: Matt Provost <[email protected]> * Update tests Signed-off-by: Matt Provost <[email protected]> --------- Signed-off-by: Matt Provost <[email protected]> Co-authored-by: Miki <[email protected]> (cherry picked from commit 4658c95)

* Add search icon to combo box Signed-off-by: Matt Provost <[email protected]> * Change from search icon to generic icon Signed-off-by: Matt Provost <[email protected]> * Update tests Signed-off-by: Matt Provost <[email protected]> --------- Signed-off-by: Matt Provost <[email protected]> Co-authored-by: Miki <[email protected]> (cherry picked from commit 4658c95) Co-authored-by: Matt Provost <[email protected]> Co-authored-by: Sean Neumann <[email protected]>

Add search icon to combo box

1619b15

Signed-off-by: Matt Provost <[email protected]>

BSFishy requested a review from a team as a code owner November 22, 2022 18:17

BSFishy requested a review from KrooshalUX November 22, 2022 18:18

BSFishy linked an issue Nov 22, 2022 that may be closed by this pull request

(OUI Component Refresh) Add prop to set OuiIcon type=search true or false #130

Closed

BSFishy self-assigned this Dec 16, 2022

BSFishy and others added 2 commits December 30, 2022 11:13

Merge branch 'opensearch-project:main' into add_combobox_icon

699e13a

Change from search icon to generic icon

e6538c8

Signed-off-by: Matt Provost <[email protected]>

BSFishy changed the title ~~Add search icon to combo box~~ Add icon to combo box Dec 30, 2022

BSFishy requested a review from ashwin-pc December 30, 2022 20:41

ashwin-pc reviewed Jan 4, 2023

View reviewed changes

src/components/combo_box/combo_box.test.tsx Show resolved Hide resolved

Update tests

bf44042

Signed-off-by: Matt Provost <[email protected]>

ashwin-pc approved these changes Jan 4, 2023

View reviewed changes

joshuarrrr reviewed Jan 6, 2023

View reviewed changes

Merge branch 'main' into add_combobox_icon

6f40087

AMoo-Miki approved these changes Feb 23, 2023

View reviewed changes

AMoo-Miki self-requested a review February 23, 2023 01:27

AMoo-Miki approved these changes Feb 23, 2023

View reviewed changes

ananzh merged commit 4658c95 into opensearch-project:main Feb 28, 2023

ananzh added the backport 1.0 label Feb 28, 2023

opensearch-trigger-bot bot mentioned this pull request Feb 28, 2023

[Backport 1.0] Add icon to combo box #537

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add icon to combo box #133

Add icon to combo box #133

BSFishy commented Nov 22, 2022

ashwin-pc commented Nov 23, 2022

KrooshalUX commented Nov 23, 2022 •

edited

Loading

ashwin-pc commented Nov 23, 2022

KrooshalUX commented Nov 27, 2022 •

edited

Loading

ashwin-pc commented Nov 29, 2022

KrooshalUX commented Nov 29, 2022 via email

kgcreative commented Nov 29, 2022

ashwin-pc commented Nov 30, 2022

kgcreative commented Nov 30, 2022 •

edited

Loading

KrooshalUX commented Nov 30, 2022 via email •

edited

Loading

kgcreative commented Nov 30, 2022

AMoo-Miki commented Dec 3, 2022

kgcreative commented Dec 3, 2022

ashwin-pc commented Dec 9, 2022

KrooshalUX commented Dec 9, 2022

BSFishy commented Dec 9, 2022

ashwin-pc commented Dec 9, 2022

ashwin-pc left a comment

joshuarrrr left a comment

joshuarrrr Jan 6, 2023

joshuarrrr Jan 6, 2023

joshuarrrr Jan 6, 2023

Add icon to combo box #133

Add icon to combo box #133

Conversation

BSFishy commented Nov 22, 2022

Description

Issues Resolved

Check List

ashwin-pc commented Nov 23, 2022

KrooshalUX commented Nov 23, 2022 • edited Loading

ashwin-pc commented Nov 23, 2022

KrooshalUX commented Nov 27, 2022 • edited Loading

ashwin-pc commented Nov 29, 2022

KrooshalUX commented Nov 29, 2022 via email

kgcreative commented Nov 29, 2022

ashwin-pc commented Nov 30, 2022

kgcreative commented Nov 30, 2022 • edited Loading

KrooshalUX commented Nov 30, 2022 via email • edited Loading

kgcreative commented Nov 30, 2022

AMoo-Miki commented Dec 3, 2022

kgcreative commented Dec 3, 2022

ashwin-pc commented Dec 9, 2022

KrooshalUX commented Dec 9, 2022

BSFishy commented Dec 9, 2022

ashwin-pc commented Dec 9, 2022

ashwin-pc left a comment

Choose a reason for hiding this comment

joshuarrrr left a comment

Choose a reason for hiding this comment

joshuarrrr Jan 6, 2023

Choose a reason for hiding this comment

joshuarrrr Jan 6, 2023

Choose a reason for hiding this comment

joshuarrrr Jan 6, 2023

Choose a reason for hiding this comment

KrooshalUX commented Nov 23, 2022 •

edited

Loading

KrooshalUX commented Nov 27, 2022 •

edited

Loading

kgcreative commented Nov 30, 2022 •

edited

Loading

KrooshalUX commented Nov 30, 2022 via email •

edited

Loading