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

style(inputsearch): change default html clear icon #607

Merged

Conversation

masoudmanson
Copy link
Contributor

@masoudmanson masoudmanson commented Sep 21, 2023

Summary

InputSearch
Github issue: #606

When text is entered into the InputSearch component, you may have noticed the default HTML clear icon. However, this default icon doesn't align with our SDS iconography. To improve consistency and visual coherence, we need to replace the default clear icon with the SDS xMark icon.

Current InputSearch:



Desired Result:



Live demo:
https://616981d9028812003ade73f3-jouthyttmu.chromatic.com/?path=/story/inputs-inputsearch--default

Checklist

  • Default Story in Storybook
  • LivePreview Story in Storybook
  • Test Story in Storybook
  • Tests written
  • Variables from defaultTheme.ts used wherever possible
  • If updating an existing component, depreciate flag has been used where necessary
  • Chromatic build verified by @chanzuckerberg/sds-design

@masoudmanson masoudmanson linked an issue Sep 21, 2023 that may be closed by this pull request
@masoudmanson masoudmanson self-assigned this Sep 21, 2023
@masoudmanson masoudmanson added the Ready for review This PR is ready for review label Sep 21, 2023
Copy link
Contributor

@tihuan tihuan left a comment

Choose a reason for hiding this comment

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

code LGTM! Will wait for @clarsen-czi on design 👍 Thanks all!

Copy link

@clarsen-czi clarsen-czi left a comment

Choose a reason for hiding this comment

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

Looks great @masoudmanson ! Is there a way to make the xMark icon a primary ButtonIcon so that it has hover and pressed effects? Thanks!

@masoudmanson
Copy link
Contributor Author

Looks great @masoudmanson ! Is there a way to make the xMark icon a primary ButtonIcon so that it has hover and pressed effects? Thanks!

I'm not sure. I've changed the default HTML clean icon for the search inputs. Let me see if I can add hover and pressed effects. 😯

@@ -147,13 +147,27 @@ const Autocomplete = <
*/
...params.InputProps.ref,
endAdornment: (
<InputAdornment position="end">
<StyledInputAdornment position="end">
Copy link
Contributor Author

@masoudmanson masoudmanson Sep 21, 2023

Choose a reason for hiding this comment

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

Because the Autocomplete component overrides the InputSearch's endAdornment, we must also include the clear IconButton here.

Copy link
Contributor

Choose a reason for hiding this comment

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

feels like it's worth adding this comment in the code!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, sure :D

@@ -193,6 +207,16 @@ const Autocomplete = <
/>
);

function clearInput() {
setInputValue("");
if (onInputChange)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we are manually firing this event, we must build a onChange event to transmit the updated value to onChange listeners.

Copy link
Contributor

Choose a reason for hiding this comment

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

let's add this context comment in the code too please 🙏

@@ -77,7 +92,20 @@ const InputSearch = forwardRef<HTMLDivElement, InputSearchProps>(
// passed to mui Input
InputProps={{
endAdornment: (
<InputAdornment position="end">
<StyledInputAdornment position="end">
{value && (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I replaced the default HTML clear icon for search inputs with an SDS xMark icon as an input adornment at the end of the input.


if (handleSubmit) handleSubmit("");

if (onChange) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as the Autocomplete component, Because we are manually firing this event, we must build a onChange event to transmit the updated value to onChange listeners.

@@ -100,8 +128,9 @@ const InputSearch = forwardRef<HTMLDivElement, InputSearchProps>(
sdsStyle={sdsStyle}
sdsStage={hasValue ? "userInput" : "default"}
onChange={handleChange}
onKeyPress={handleKeyPress}
onKeyDown={handleKeyPress}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The onKeyPress event has been deprecated and replaced by the newer onKeyDown event.

intent={intent}
autoComplete="off"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This prevents the browser's default auto completion menu from being displayed for the InputSearch.

Screenshot 2023-09-21 at 5 08 16 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add this comment in the code too for context!

@@ -116,14 +117,30 @@ export const StyledSearchBase = styled(TextField, {
min-width: 120px;
display: block;

[type="search"]::-webkit-search-cancel-button {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part hides the default HTML clear icon.

Copy link

@clarsen-czi clarsen-czi left a comment

Choose a reason for hiding this comment

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

For some reason the hover effect on the new xmark icon button doesn't look like it's working; when I hover over it no color change occurs.

@masoudmanson
Copy link
Contributor Author

For some reason the hover effect on the new xmark icon button doesn't look like it's working; when I hover over it no color change occurs.

Hmm, 🤔
It must be because of the position: absolute;
I'll fix it 😊

Copy link

@clarsen-czi clarsen-czi left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @masoudmanson !

@masoudmanson masoudmanson merged commit dddcd34 into main Sep 22, 2023
7 checks passed
@masoudmanson masoudmanson deleted the 606-replace-inputsearchs-clear-icon-with-sds-xmark-icon branch September 22, 2023 18:40
@masoudmanson masoudmanson added Released and removed Ready for review This PR is ready for review labels Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace InputSearch's Clear Icon with SDS xMark Icon
3 participants