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

[Autocomplete] Provide a way to remove the search icon #11224

Closed
2 of 6 tasks
driskull opened this issue Jan 8, 2025 · 13 comments
Closed
2 of 6 tasks

[Autocomplete] Provide a way to remove the search icon #11224

driskull opened this issue Jan 8, 2025 · 13 comments
Assignees
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Maps SDK for JavaScript Issues logged by ArcGIS SDK for JavaScript team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. calcite-components Issues specific to the @esri/calcite-components package. estimate - 3 A day or two of work, likely requires updates to tests. impact - p1 - need for current milestone User set priority impact status of p1 - need for current milestone p - medium Issue is non core or affecting less that 60% of people using the library

Comments

@driskull
Copy link
Member

driskull commented Jan 8, 2025

Check existing issues

Actual Behavior

Currently, the search icon cannot be removed.

I wonder if we should just not set the icon like default just as the input component does? That way, there's no icon by default and a developer doesn't have to set the icon to false using JS.

Because we set the input type to search in the autocomplete component, when a user sets the icon to an empty string (icon="") it will default to the search icon because the input component defaults to the input type icon. Maybe it shouldn't? Its kind of odd because I would expect setting an icon prop to "" to remove the icon.

See https://codepen.io/Matt-Driscoll/pen/YPKYbZK?editors=1010

@jcfranco @ashetland @SkyeSeitz @matgalla would love your thoughts here.

Expected Behavior

The ability to remove the search icon.

Reproduction Sample

https://codepen.io/Matt-Driscoll/pen/XJrVwpB?editors=1010

Reproduction Steps

  1. open the codepen
  2. notice the search icon remains even though the prop has been set to false
  3. setting the prop to an empty string has no effect either

Reproduction Version

3.0.0-90

Relevant Info

questions:

  1. Should we have no default icon set for autocomplete?
  2. Should input reconsider setting the icon to the input's type when the icon prop is set to an empty string? cc @eriklharper

Regression?

No response

Priority impact

impact - p1 - need for current milestone

Impact

No response

Calcite package

  • @esri/calcite-components
  • @esri/calcite-components-react
  • @esri/calcite-design-tokens
  • @esri/calcite-ui-icons
  • @esri/eslint-plugin-calcite-components

Esri team

ArcGIS Maps SDK for JavaScript

@driskull driskull added bug Bug reports for broken functionality. Issues should include a reproduction of the bug. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Jan 8, 2025
@github-actions github-actions bot added ArcGIS Maps SDK for JavaScript Issues logged by ArcGIS SDK for JavaScript team members. calcite-components Issues specific to the @esri/calcite-components package. impact - p1 - need for current milestone User set priority impact status of p1 - need for current milestone labels Jan 8, 2025
@driskull
Copy link
Member Author

driskull commented Jan 8, 2025

setRequestedIcon is also weird because it will use the default icon for the type if its an empty string but not null or undefined.

export function setRequestedIcon(
iconObject: Record<string, IconNameOrString>,
iconValue: IconNameOrString | boolean | "",
matchedValue: string,
): IconNameOrString | undefined {
if (typeof iconValue === "string" && iconValue !== "") {
return iconValue;
} else if (iconValue === "" || iconValue === true) {
return iconObject[matchedValue];
}
}

@jcfranco
Copy link
Member

jcfranco commented Jan 8, 2025

IIRC, the icon prop (boolean | string) should follow this behavior:

  1. false should remove the icon
  2. true uses whatever icon is associated with the type
  3. string must match a supported icon

There might be some inconsistencies or follow-up needed due to Stencil's quirks with props of mixed type.

@jcfranco jcfranco added 1 - assigned Issues that are assigned to a sprint and a team member. estimate - 3 A day or two of work, likely requires updates to tests. p - medium Issue is non core or affecting less that 60% of people using the library and removed 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Jan 8, 2025
@ashetland
Copy link
Contributor

IIRC, the icon prop (boolean | string) should follow this behavior:

  1. false should remove the icon
  2. true uses whatever icon is associated with the type
  3. string must match a supported icon

There might be some inconsistencies or follow-up needed due to Stencil's quirks with props of mixed type.

Agree with this behavior. FWIW, the Figma version has a toggle for designers to turn the icon on/off (at the time, I think I assumed developers could do the same 😆). We're setting the default to icon="search", but we also provide an easy way for designers to change the icon in the list of properties.

I also wanted to point out that Combobox is also different in this regard. It has a placeholder-icon prop that, when used, shows an icon, but then hides it upon selection. I don't think we should adopt this behavior in Autocomplete, but I'm curious about the history there and wonder if we could be more consistent with this across the board?

@macandcheese
Copy link
Contributor

Some type of input have default icon - which you can opt into with the boolean, and changes depending on "type" value. Combobox doesn't have that "type" property, so there wouldn't necessarily be a default icon there - "placeholder-icon" gets replaced with a selected Combobox Item's "icon" prop (if available), which is why we opted for a distinct name there (vs. "icon")

@driskull
Copy link
Member Author

driskull commented Jan 8, 2025

  1. false should remove the icon
  2. true uses whatever icon is associated with the type
  3. string must match a supported icon

Those make sense, this also happens though:

  1. "" uses whatever icon is associated with the type

It just makes it a little weird for a user to remove the icon. They have to specifically use false because the empty string won't work as it will default to the type icon

@driskull
Copy link
Member Author

driskull commented Jan 8, 2025

@jcfranco another problem is that the an undefined value is inconsistent between input and notice.

  • In input, when there is a type defined and icon is undefined it expects to use the default icon for the type.
  • In notice when there is a kind defined and icon is undefined it expects no icon to be shown.

Should we proceed with these inconsistencies and update the utility to handle both cases or should we make changes to be consistent between them?

@jcfranco
Copy link
Member

jcfranco commented Jan 8, 2025

It just makes it a little weird for a user to remove the icon. They have to specifically use false because the empty string won't work as it will default to the type icon

This is expected and the correct behavior. IIRC, this was caused by Stencil's handling of mixed types.

Side note: The internal 3.0 breaking change release doc and the breaking change notes in PR #10482 touch on this. After re-reading, we might want to add more detail to clarify.

In input, when there is a type defined and icon is undefined it expects to use the default icon for the type.

This seems like a bug. According to the doc and previously described behavior, this shouldn't display an icon, right?

@driskull
Copy link
Member Author

driskull commented Jan 8, 2025

This seems like a bug. According to the doc and previously described behavior, this shouldn't display an icon, right?

Ok, so undefined and "" should both show no icon?

@jcfranco
Copy link
Member

jcfranco commented Jan 8, 2025

Correct; falsy values should not show an icon.

@driskull
Copy link
Member Author

driskull commented Jan 9, 2025

this is one rabbit hole 🕳️

Because we have CSS using :host([icon]) all these styles are getting applied when the component has icon="". 👎

:host([icon][scale="s"]) input {
padding-inline-start: theme("padding.8");
}
:host([icon][scale="m"]) input {
padding-inline-start: theme("padding.10");
}
:host([icon][scale="l"]) input {
padding-inline-start: theme("padding.14");
}

Should I fix as part of this or follow up issue @jcfranco?

@driskull
Copy link
Member Author

driskull commented Jan 9, 2025

Another issue: When we have icon: IconNameOrString | boolean and we use HTML like the following:

<calcite-alert icon></calcite-alert>

When we get the value of alert.icon it is a string value of "" which means we need to treat "" as true, right?

So we can't really treat "" as a false value. @jcfranco

Its being treated as true in 2x and 3x currently.
https://codepen.io/Matt-Driscoll/pen/ogvEwGz?editors=100

driskull added a commit that referenced this issue Jan 9, 2025
**Related Issue:** #11224

## Summary

- allow icon to be set false
- add tests

BEGIN_COMMIT_OVERRIDE
END_COMMIT_OVERRIDE
@driskull driskull added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 1 - assigned Issues that are assigned to a sprint and a team member. labels Jan 9, 2025
@github-actions github-actions bot assigned geospatialem and DitwanP and unassigned driskull Jan 9, 2025
Copy link
Contributor

github-actions bot commented Jan 9, 2025

Installed and assigned for verification.

@geospatialem geospatialem added 4 - verified Issues that have been released and confirmed resolved. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Jan 10, 2025
@geospatialem
Copy link
Member

Verified with 3.0.0-next.93 with:

<!-- Custom icon -->
<calcite-autocomplete icon="banana"></calcite-autocomplete>

<!-- No icon -->
<calcite-autocomplete id="autocompleteNoIcon"></calcite-autocomplete>
document.getElementById("autocompleteNoIcon").icon = false;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Maps SDK for JavaScript Issues logged by ArcGIS SDK for JavaScript team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. calcite-components Issues specific to the @esri/calcite-components package. estimate - 3 A day or two of work, likely requires updates to tests. impact - p1 - need for current milestone User set priority impact status of p1 - need for current milestone p - medium Issue is non core or affecting less that 60% of people using the library
Projects
None yet
Development

No branches or pull requests

6 participants