TextInput action hover state is incorrect

[maximedegreve]

Description

The hover state for clearing an input should just slightly change the icon color instead of providing a background behind it. The background right now is also incorrectly sized making it even more apparent that it's implementation is incorrect.

Additionally the example in storybook shows the clear button even though there is no input value. This is incorrect behaviour and could encourage a wrong pattern. It's crucial that the clear button is hidden when the input is empty. Therefore I suggest adding this simple functionality in storybook and be default add a value to it.

Steps to reproduce

1. Go to https://primer.style/react/storybook/?path=/story/components-textinput-features--with-trailing-action
2. Hover the clear button

Version

v35.32.1

Browser

Safari

[github-actions[bot]]

Uh oh! @maximedegreve, the image you shared is missing helpful alt text. Check your issue body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

[cs25-esc]

hi ! can you assign this to me? i know the fix!!

[lesliecdubs]

Sure thing @cs25-esc, sending this your way! For reference, here's a link to the contribution guidelines for PRC. Let us know if run into questions.

[cs25-esc]

sure thanks @lesliecdubs

[cs25-esc]

Hi @lesliecdubs i could solve the issue "clear button will not be displayed when there is no input" checking other

[cs25-esc]

Hi @lesliecdubs / @maximedegreve fixed both the issues on my local i have a doubt like : on hovering the clear input icon you said it should change the icon color slightly (what does it mean, shall i change the opacity of the icon or altogether a different color?)

[cs25-esc]

https://github.com/primer/react/assets/68850280/da93f4c7-d8c5-431e-a9c3-7dcee666cfd7

@lesliecdubs / @maximedegreve can you please verify this

[maximedegreve]

This looks good @cs25-esc 🎉

[cs25-esc]

Hi @maximedegreve / @lesliecdubs i have submitted my PR for this issue.

[siddharthkp]

Additionally the example in storybook shows the clear button even though there is no input value. This is incorrect behaviour and could encourage a wrong pattern.

Hi @maximedegreve and @cs25-esc!

First of all, valid bug!

But I don't think we should hide it from this specific story, because this story shows how to add a trailing action (could be clear, but could also be show password (eyes), copy, etc.). It would be weird if in the trailing action example, there is no trailing example by default.

We can change the action used in this story to a different action that's always visible and (optionally) add another story that shows clear action example.

[siddharthkp]

There are 3 parts of this issue:

1. The hover state for clearing an input should just slightly change the icon color instead of providing a background behind it.

   We use an IconButton with variant=invisible as the action, which comes with it's own hover styles. We can change component styles for trailing action for that.

   @maximedegreve Should open another issue for PVC as well

2. Our trailing action example in storybook doesn't match our own design guidelines.

   This is incorrect behaviour and could encourage a wrong pattern. It's crucial that the clear button is hidden when the input is empty.

   Because we want to show trailing action in the example, what we can do here is add a default value to the input to show the action and then hide it if you remove the text. To make this story even clearer, we can add two text inputs, one with text and one without.

3. clear action should only appear when there is content inside the input, but other actions might be visible from the start.

   This is the tricky part. Right now, it's up to the application to handle that. We should probably bake this behavior into the component for clear. PVC has an attibute called show_clear_button, works but not a big fan. Needs more thought.

[lesliecdubs]

@cs25-esc thanks for helping out with this! Would you be willing to replicate the same change to the hover state style in Primer ViewComponents https://github.com/primer/view_components as well?

[cs25-esc]

@cs25-esc thanks for helping out with this! Would you be willing to replicate the same change to the hover state style in Primer ViewComponents https://github.com/primer/view_components as well?

yeah sure let me check.

[cs25-esc]

@lesliecdubs I'm thinking like let me wait for the PR - https://github.com/primer/react/pull/3894 to get done before making the same changes for Primer ViewComponents.

[lesliecdubs]

That sounds reasonable to me, @cs25-esc! I'm going to leave this issue assigned to you for now; just let us know if you aren't able to come back to it.

[dusave]

@cs25-esc @lesliecdubs Circling back on this thread, did this get fixed from the Primer side?

[lesliecdubs]

@dusave looks like the associated PR that was in progress https://github.com/primer/react/pull/3894 didn't end up getting merged. @siddharthkp would reviving https://github.com/primer/react/pull/3894 for reviews be a big lift?

[siddharthkp]

• Revived in https://github.com/primer/react/pull/4831