Text Input enhancements #1661

pksjce · 2021-11-29T10:30:37Z

Describe your changes here.

Closes # https://github.com/github/primer/issues/501

Screenshots

Some additions

Error condition

Warning condition

Trailing Icon

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.

changeset-bot · 2021-11-29T10:30:40Z

🦋 Changeset detected

Latest commit: 34a05fa

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	Patch

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

github-actions · 2021-11-29T10:34:30Z

size-limit report 📦

Path	Size
dist/browser.esm.js	57.5 KB (+0.41% 🔺)
dist/browser.umd.js	57.8 KB (+0.37% 🔺)

colebemis · 2021-11-29T20:17:19Z

src/TextInput.tsx

+  // deprecate icon prop
  icon?: React.ComponentType<{className?: string}>


I think using a JSDoc comment here will give us some nice in-editor warnings for free.

Suggested change

// deprecate icon prop

icon?: React.ComponentType<{className?: string}>

/** @deprecated Use `leadingIcon` prop instead */

icon?: React.ComponentType<{className?: string}>

Nice will do that!

src/TextInput.tsx

colebemis

What do you think of having a composable API for leading and trailing visuals similar to action list? Maybe something like this:

<InputGroup>
  <InputGroup.LeadingVisual><SearchIcon /></InputGroup.LeadingVisual>
  <TextInput placeholder="Search" />
</InputGroup>

This is similar to Chakra UI's API: https://chakra-ui.com/docs/form/input#add-elements-inside-input

cc @siddharthkp

pksjce · 2021-11-30T07:13:23Z

Yep! Definitely looks like the composite component is our way forward seeing all the other component implementations too. My only qualm is that it gets verbose when composed at higher levels. If its a pattern we'll follow then our users should probably get used to it.
However, in this case I'm only fixing up current issues with TextInput. Do you think we want to change this api for beta?

siddharthkp · 2021-11-30T11:29:30Z

What do you think of having a composable API for leading and trailing visuals similar to action list?

<InputGroup>
  <InputGroup.LeadingVisual><SearchIcon /></InputGroup.LeadingVisual>
  <TextInput placeholder="Search" />
</InputGroup>

What's the benefit of using that API here?

Yep! Definitely looks like the composite component is our way forward seeing all the other component implementations too. My only qualm is that it gets verbose when composed at higher levels.

I agree with @pksjce here, the additional verbosity doesn't seem to give us any benefits here.

I don't think composite/compound components should not be our go-to API choice. They are useful when we're composing application components within primer components with. In TextInput, we tightly control "how" the Icon renders (size, color, spacing, etc) so <TextInput leadingVisual={SearchIcon} makes a lot of sense to me.

src/TextInput.tsx

mperrotti · 2021-11-30T19:55:01Z

src/TextInput.tsx

  icon?: React.ComponentType<{className?: string}>
+  leadingIcon?: React.ComponentType<{className?: string}>
+  trailingIcon?: React.ComponentType<{className?: string}>


Same feedback as the leading icon, but I think it's even more likely we'll use this space for things besides icons. Here's an example from Codespaces settings where we're using this space to show a "minutes" unit:

Another thing to consider for this space: we might want to render a button inside the input. For example: in the project board search, we show a button to clear the input.

Will update to trailingVisual property for the first comment.

Yes, I see in this

from https://github.com/github/primer/issues/156
I was wondering if your work in https://github.com/primer/react/pull/1611/files would cover TextInput with actions. I'm still reviewing your PR to find that out.

No, my work in #1611 doesn't touch anything inside the inputs. It just handles the layout and passing the correct attributes for screenreader accessibility (e.g.: matching the label's for attribute to the input's id attribute)

Hmmm then I think we should discuss this in a separate PR. It's common to have trailingVisual and trailingAction in our components. Possibly we can think of adding trailingAction onto the TextInput?

I'm happy with addressing this in a separate PR. Agreed - trailingAction should be on the TextInput

mperrotti · 2021-11-30T20:02:53Z

src/TextInput.tsx

        contrast={contrast}
        disabled={disabled}
-        hasIcon={!!IconComponent}
+        hasIcon={!!IconComponent || !!(LeadingIconComponent || TrailingIconComponent)}


Non-blocking nitpick: I prefer Boolean(isTruthy) over !!isTruthy just because I find it more readable.

Removed this check. It was not required after all

mperrotti · 2021-11-30T20:16:24Z

src/_TextInputWrapper.tsx

@@ -25,45 +43,50 @@ type StyledWrapperProps = {
  hasIcon?: boolean
  block?: boolean
  contrast?: boolean
+  validationStatus?: 'error' | 'warning'


Heads up: I defined this as a type in utils/types in one of my PRs: https://github.com/primer/react/pull/1611/files#diff-410797b1c4259d74820358a792a62f83cfc7a1c0a0eb26226c89e4621b88d42cR1

I didn't include 'warning' because I couldn't figure out where we use it, but I'm fine with keeping it.

Does that mean I have to handle success case in TextInput?

We don't have a special style for inputs with a validation status of success. We just style any success message we might show.

For example:

Isn't it unnecessary for me to use that type then since it has success in it too.

mperrotti · 2021-11-30T20:32:21Z

src/_TextInputWrapper.tsx

  background-repeat: no-repeat; // Repeat and position set for form states (success, error, etc)
  background-position: right 8px center; // For form validation. This keeps images 8px from right and centered vertically.
  border: 1px solid ${get('colors.border.default')};
  border-radius: ${get('radii.2')};
  outline: none;
  box-shadow: ${get('shadows.primer.shadow.inset')};
  cursor: text;
+  padding: 6px 12px;
+  display: grid;
+  grid-template-areas: 'leadingIcon input trailingIcon';


We should add grid-template-columns: auto 1fr auto; so that the empty grid cells collapse when no content is passed.

Stuff like this will happen if we don't:

Yeah, I've been trying to fix this as well.
But your solution results in

Adding

grid-template-columns: auto 1fr auto; justify-items: end;

seems to help. WDYT?

It looks like the text input is getting sized with auto when it's the first child. We might need to change the grid-template-columns based on what children are there.

leadingVisual + input + trailingVisual

grid-template-columns: auto 1fr auto;

leadingVisual + input

grid-template-columns: auto 1fr;

input + trailingVisual

grid-template-columns: 1fr auto;

input

grid-template-columns: 1fr;
or
grid-template-columns: unset;

I was hoping grid would have some fancy way of handling this without having to specify different column templates for each case, but I couldn't figure it out :(

Maybe another reviewer will know?

Or maybe we could just use flexbox instead of grid? Is there something we need from grid that can't be done with flexbox?

Yeah I'm not particularly attached to the grid way. Do you want to pair on this for a bit tomorrow?

Sounds great! I love a CSS pairing sesh :)

vdepizzol · 2021-12-06T23:24:56Z

src/_TextInputWrapper.tsx

  background-repeat: no-repeat; // Repeat and position set for form states (success, error, etc)
  background-position: right 8px center; // For form validation. This keeps images 8px from right and centered vertically.
  border: 1px solid ${get('colors.border.default')};
  border-radius: ${get('radii.2')};
  outline: none;
  box-shadow: ${get('shadows.primer.shadow.inset')};
  cursor: text;
+  padding: 6px 12px;


i'm afraid there's too much horizontal padding for a text input in here. We should aim for more compact UIs where possible 😇.

Suggested change

padding: 6px 12px;

padding: 6px 8px;

Also, because this padding is being set in the wrapper, and not in the <input> itself, the surroundings of the input element aren't interactive.

Screen.Recording.2021-12-06.at.15.27.53.mov

We should make sure the <input> area covers as much room as possible (saving for leading and trailing elements) within the wrapper.

One last thing :)

The vertical padding of 6px is ignoring the border, resulting in an input field height of 34px¹. We should make sure the component height as a whole matches 32px.

🙇

Footnotes

20px of line height, 6px+6px of vertical padding, 1px+1px of border ↩

This is great feedback! Had not considered the area covered by the input. Fixing this right away.

Edit - Looking more deeply into this, the wrapper is the same as the one used in TextInputWithTokens. The height on that component is a minimum of 34px and the padding is the same as this component. Do you have similar feedback on that component as well? Or should i work on having independent logic and not reuse whats been done there?
https://primer.style/react/storybook?path=/story/forms-text-input-with-tokens--default

cc - @mperrotti

The height on that component is a minimum of 34px and the padding is the same as this component.

We should probably fix it in there too. We should make sure the form components match sizes with buttons at a 32px height for the default size variant. c/c @mperrotti

🙇

@vdepizzol - ~~I'm going to take care of this in a follow-up PR. It's not a trivial amount of work to add to these changes.~~

Edit: this was actually much simpler than I was expecting, so I added the changes in this PR.

cc @pksjce

mperrotti

Everything is looking good! After @pksjce and I pair on the layout of the leading/trailing visuals and the input, this should be ready to merge.

mperrotti

Add a changeset, and then it's good to go!

2. Change grid logic 3. Update stories and docs

* Enhance text input component * Add tests * Fix up review comments * 1. Add leading and trailing visual 2. Change grid logic 3. Update stories and docs * Fix up documentation * Update documentation and css layout * fixes text input height and padding * fixes spacing with leading and trailing visuals * Lint issues * Create clever-shrimps-search.md Co-authored-by: Mike Perrotti <mperrotti@github.com>

pksjce requested review from mperrotti, rezrah and a team November 29, 2021 10:30

github-actions bot deployed to Preview November 29, 2021 10:42 View deployment

github-actions bot deployed to Preview November 29, 2021 10:45 View deployment

colebemis reviewed Nov 29, 2021

View reviewed changes

src/TextInput.tsx Outdated Show resolved Hide resolved

colebemis reviewed Nov 29, 2021

View reviewed changes

src/TextInput.tsx Outdated Show resolved Hide resolved

colebemis reviewed Nov 29, 2021

View reviewed changes

pksjce force-pushed the pk/text-input-enhancements branch from 5985748 to 971850d Compare November 30, 2021 08:20

github-actions bot deployed to Preview November 30, 2021 08:21 View deployment

mperrotti reviewed Nov 30, 2021

View reviewed changes

pksjce force-pushed the pk/text-input-enhancements branch from 971850d to a3b0824 Compare December 1, 2021 09:40

github-actions bot deployed to Preview December 1, 2021 09:40 View deployment

github-actions bot deployed to Preview December 1, 2021 09:42 View deployment

github-actions bot deployed to Preview December 2, 2021 04:56 View deployment

pksjce added the react label Dec 2, 2021

vdepizzol reviewed Dec 6, 2021

View reviewed changes

pksjce force-pushed the pk/text-input-enhancements branch from 40bfb3e to bfead25 Compare December 9, 2021 06:03

github-actions bot deployed to Preview December 9, 2021 06:03 View deployment

mperrotti reviewed Dec 9, 2021

View reviewed changes

github-actions bot deployed to Preview December 13, 2021 15:43 View deployment

pksjce force-pushed the pk/text-input-enhancements branch from c59e741 to 51c7fbf Compare December 14, 2021 01:52

github-actions bot deployed to Preview December 14, 2021 01:52 View deployment

github-actions bot deployed to Preview December 14, 2021 04:50 View deployment

pksjce force-pushed the pk/text-input-enhancements branch from e882130 to 187d441 Compare December 14, 2021 04:51

github-actions bot deployed to Preview December 14, 2021 04:52 View deployment

mperrotti approved these changes Dec 14, 2021

View reviewed changes

github-actions bot deployed to Preview December 14, 2021 22:59 View deployment

github-actions bot deployed to Preview December 14, 2021 23:25 View deployment

pksjce force-pushed the pk/text-input-enhancements branch from d1a7b0e to c959444 Compare December 15, 2021 04:49

github-actions bot deployed to Preview December 15, 2021 04:49 View deployment

github-actions bot deployed to Preview December 15, 2021 04:50 View deployment

pksjce force-pushed the pk/text-input-enhancements branch from 9ca2335 to b179fad Compare December 15, 2021 04:59

github-actions bot deployed to Preview December 15, 2021 04:59 View deployment

github-actions bot deployed to Preview December 15, 2021 05:05 View deployment

pksjce and others added 9 commits December 15, 2021 16:05

Enhance text input component

75bf0c6

Add tests

2b357ca

Fix up review comments

f3f9d6e

1. Add leading and trailing visual

6c3162c

2. Change grid logic 3. Update stories and docs

Fix up documentation

b28af3e

Update documentation and css layout

a4d301a

fixes text input height and padding

7ba5b64

fixes spacing with leading and trailing visuals

e92dc07

Lint issues

b6f2f6c

pksjce force-pushed the pk/text-input-enhancements branch from a1906bb to b6f2f6c Compare December 15, 2021 05:05

github-actions bot deployed to Preview December 15, 2021 05:06 View deployment

Create clever-shrimps-search.md

34a05fa

github-actions bot deployed to Preview December 15, 2021 05:08 View deployment

pksjce merged commit 6f13441 into main Dec 15, 2021

pksjce deleted the pk/text-input-enhancements branch December 15, 2021 05:19

mperrotti mentioned this pull request Dec 15, 2021

Select input component #1736

Merged

6 tasks

primer-css mentioned this pull request Dec 15, 2021

Release Tracking #1707

Merged

pksjce mentioned this pull request Jan 14, 2022

TextInput bg colour not being applied #1142

Closed

		// deprecate icon prop
		icon?: React.ComponentType<{className?: string}>

Text Input enhancements #1661

Text Input enhancements #1661

Uh oh!

Conversation

pksjce commented Nov 29, 2021

Screenshots

Merge checklist

Uh oh!

changeset-bot bot commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

colebemis left a comment • 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!

pksjce commented Nov 30, 2021

Uh oh!

siddharthkp commented Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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!

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!

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

leadingVisual + input + trailingVisual

leadingVisual + input

input + trailingVisual

input

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!

changeset-bot bot commented Nov 29, 2021 •

edited

Loading

github-actions bot commented Nov 29, 2021 •

edited

Loading

colebemis left a comment •

edited

Loading

siddharthkp commented Nov 30, 2021 •

edited

Loading

`leadingVisual` + input + `trailingVisual`

`leadingVisual` + input

input + `trailingVisual`

pksjce Dec 8, 2021 •

edited

Loading

vdepizzol Dec 14, 2021 •

edited

Loading

mperrotti Dec 14, 2021 •

edited

Loading