Add trailingAction to TextInput by mperrotti · Pull Request #1947 · primer/react

mperrotti · 2022-03-10T14:44:24Z

Adds the capability to render a trailing action in a text input.

Closeshttps://github.com/github/primer/issues/748

Screenshots

Merge checklist

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

…ding-state

Co-authored-by: Pavithra Kodmad <pksjce@github.com>

… into mp/text-input-loading-state

changeset-bot · 2022-03-10T14:44:27Z

🦋 Changeset detected

Latest commit: e73b569

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

mperrotti · 2022-03-10T14:47:35Z

#1920 should merge before this

…ding-state

mperrotti · 2022-03-21T20:10:23Z


+// TODO: figure out how to type a themed CSS function (e.g.: css`color: blue;`)
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+const renderFocusStyles = (hasActions: boolean, isInputFocused: boolean, focusStyles: any) => {


Help wanted: what's a good way to write the correct type for the result of a themed CSS function? For example: (e.g.: css`color: blue;`)

@siddharthkp or @colebemis - any ideas?

colebemis · 2022-03-21T20:30:46Z

+ <TextInput
+ trailingAction={
+ <TextInput.Action
+ onClick={() => {
+ alert('clear input')
+ }}
+ icon={XIcon}
+ iconLabel="Clear input"
+ sx={{color: 'fg.subtle'}}
+ />
+ }
+ />


I'm not super confident about this but what do you think about an API like this:
Suggested change
<TextInput
trailingAction={
<TextInput.Action
onClick={() => {
alert('clear input')
}}
icon={XIcon}
iconLabel="Clear input"
sx={{color: 'fg.subtle'}}
/>
}
/>
<TextInput>
<TextInput.TrailingAction
onClick={() => {
alert('clear input')
}}
icon={XIcon}
iconLabel="Clear input"
sx={{color: 'fg.subtle'}}
/>
</TextInput>
cc @siddharthkp

Oh yeah! Me and @mperrotti went through a bunch of options and this was one of them!
What I like:
It makes TextInput feel like a compound component and then putting children inside could makes sense
What I don't like:
Doesn't match the prop for trailingVisual (we could move that inside as well along with leadingVisual)
It's not intuitive to put contents inside a TextInput because input can't have children, it's hard to predict what the output of this would be.

@siddharthkp said exactly what I was going to 😅

colebemis · 2022-03-21T20:32:59Z

+ <TextInput.Action
+ onClick={() => {
+ alert('clear input')
+ }}
+ icon={XIcon}
+ iconLabel="Clear input"
+ tooltipMessage="Clear"
+ sx={{color: 'fg.subtle'}}
+ />


Is there a reason we couldn't wrap TextInput.Action with a Tooltip?
<Tooltiparia-label="Clear"><TextInput.Action.../></Tooltip>

Or... should we have a strong opinion about tooltips and automatically make the icon label visible in a tooltip?

I imagine consumers don't to want to think about when they should use a tooltip

+1 to always automatically tooltipped
also, would iconLabel and tooltipMessage be different?

I vote we replace iconLabel and tooltipMessage with an aria-label prop that is required if no children are passed

@siddharthkp - I thought there would be a case where the iconLabel and tooltipMessage are different, but now I'm second-guessing that... 🤔
I'm going to move forward with @colebemis's suggestion, and we can change that later if we need to.

Now that I'm implementing this, I'm not sure aria-label is an intuitive prop to use for the tooltip message. Especially for buttons with a text label.
For example, this feels a little strange:
<TextInput.Actionaria-label="Show password">Show</TextInput.Action>
I think this is a weird enough use-case that we can just go forward with Cole's suggestion, but I wanted to bring this up.

Should we start without allowing a tooltip for buttons with text labels? This feels like a reasonable limitation, but I don't like that we would just silently swallow the aria-label

Should we start without allowing a tooltip for buttons with text labels? This feels like a reasonable limitation
That sounds reasonable!
, but I don't like that we would just silently swallow the aria-label
For actions with text labels, is there a reason not to pass aria-label through to the Button if it's provided?

I decided not to limit it

…iling-action

siddharthkp

Only one tiny piece left: #1947 (comment)

…iling-action

…ct into mp/text-input-trailing-action

mperrottiand others added 13 commits March 2, 2022 10:49

adds loading indicator to text inputs
c9f34f1

removes style debugging div
26976c7

updates stories and docs
5157087

updates tests
258ed6c

fixes silly test mistakes
245b568

revert default textinput story
6c1f358

adds changeset
81a1022

Merge branch 'main' into mp/text-input-loading-state
89b0e44

Merge branch 'main' of github.com:primer/react into mp/text-input-loa…
e964c11
…ding-state

Update src/TextInput.tsx
5388500
Co-authored-by: Pavithra Kodmad <pksjce@github.com>

Merge branch 'mp/text-input-loading-state' of github.com:primer/react…
0473333
… into mp/text-input-loading-state

updates component API
070612f

adds trailing action to text input, tweaks formcontrol
097e8fb

mperrotti requested review from a team and colebemis March 10, 2022 14:44

mperrotti marked this pull request as draft March 10, 2022 14:44

mperrotti added 6 commits March 10, 2022 11:43

Merge branch 'main' of github.com:primer/react into mp/text-input-loa…
f2f52c7
…ding-state

addresses PR feedback
79fbe76

fixes linting error
4a4fcc0

indicate a busy status to assistive technology
1bdf6d1

Merge branch 'main' of github.com:primer/react into mp/text-input-loa…
5f14db8
…ding-state

updates snaps
3464982

mperrotti mentioned this pull request Mar 11, 2022
feat: bumps autocomplete element package, updates API primer/view_components#1050
Merged

mperrotti added the react label Mar 11, 2022

mperrotti added 2 commits March 15, 2022 13:12

renames 'isLoading' prop to 'loading'
d586a80

Merge branch 'main' of github.com:primer/react into mp/text-input-loa…
ced3de3
…ding-state

rezrah requested a review from siddharthkp March 21, 2022 15:38

siddharthkp self-assigned this Mar 21, 2022

silences linting error and rms bad imports
e94c13c

mperrotti commented Mar 21, 2022
View reviewed changes

colebemis reviewed Mar 21, 2022
View reviewed changes

Comment threadsrc/_TextInputInnerAction.tsx Outdated

siddharthkp reviewed Mar 22, 2022
View reviewed changes

Comment threadsrc/_TextInputInnerAction.tsx Outdated

mperrotti added 4 commits March 24, 2022 11:02

Merge branch 'main' of github.com:primer/react into mp/text-input-tra…
2781e4c
…iling-action

addresses PR feedback
46f1daf

rms redundant stories
0dabb66

removes excess right padding when we pass 'trailingAction' and 'loading'
5781c4d

mperrotti requested review from colebemis and siddharthkp March 24, 2022 17:10

Merge branch 'main' into mp/text-input-trailing-action
12c5f76

mperrotti temporarily deployed to visual-testing March 24, 2022 22:46 Inactive

siddharthkp approved these changes Mar 25, 2022
View reviewed changes

mperrotti added 4 commits March 25, 2022 12:46

change iconLabel to aria-label in docs example
b26b765

Merge branch 'main' of github.com:primer/react into mp/text-input-tra…
f573158
…iling-action

Merge branch 'mp/text-input-trailing-action' of github.com:primer/rea…
1339d9d
…ct into mp/text-input-trailing-action

empy commit to make CI run
6d1e442

mperrotti temporarily deployed to visual-testing March 25, 2022 18:17 Inactive

Merge branch 'main' into mp/text-input-trailing-action
641a276

mperrotti temporarily deployed to visual-testing March 29, 2022 14:21 Inactive

mperrotti added 2 commits March 29, 2022 11:24

disable html-addon to prevent Storybook from timing out Chromatic
b1d60a2

Merge branch 'mp/text-input-trailing-action' of github.com:primer/rea…
a16fb1b
…ct into mp/text-input-trailing-action

mperrotti temporarily deployed to visual-testing March 29, 2022 15:24 Inactive

Merge branch 'main' into mp/text-input-trailing-action
e73b569

mperrotti temporarily deployed to visual-testing March 29, 2022 15:25 Inactive

mperrotti merged commit edc85c9 into mainMar 29, 2022

mperrotti deleted the mp/text-input-trailing-action branch March 29, 2022 20:01

primer-css mentioned this pull request Mar 29, 2022
Release tracking #1993
Merged

Uh oh!

Conversation

mperrotti commented Mar 10, 2022

Screenshots

Merge checklist

Uh oh!

changeset-botBot commented Mar 10, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

mperrotti commented Mar 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colebemisMar 21, 2022• 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!

siddharthkpMar 22, 2022• 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colebemisMar 21, 2022• 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!

Choose a reason for hiding this comment

Uh oh!

siddharthkpMar 22, 2022• 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!

colebemisMar 22, 2022• 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

siddharthkp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

changeset-botBot commented Mar 10, 2022•
edited
Loading

colebemisMar 21, 2022•
edited
Loading

siddharthkpMar 22, 2022•
edited
Loading

colebemisMar 21, 2022•
edited
Loading

siddharthkpMar 22, 2022•
edited
Loading

colebemisMar 22, 2022•
edited
Loading