AP-544 Implement Text Inputs #45

mayakoneval · 2019-08-03T00:34:21Z

This PR includes:

Text Inputs
Steppers
Text Inputs with icons

Input Props

Points of conversation:

The Stepper onClick accepts a number, while the Input onClick accepts an HTMLInputEvent. This is because the Stepper should only call onChange when the number has actually changed, not when the user makes an action in the input. For example, they could try to decrease the number but the number is at min already, so we don't want to call onClick.
The Stepper is copy pasted from Tim's implementation and then emotionified. However, I took the responsibility of updating the value away from the consumer and into the component. @timbotnik curious as to what you think of this.
The PR does not include input masking, as Recurly does that for us when we need it.
@adamatorres the error and info states look like this (not filled in icons)

Is that okay? Note the icons are bottom aligned with the text, do you want the icons aligned display:flex style with the text so that when it wraps the icon is centered to the text like this?

Or would you rather it be in line with the text?

cheapsteak

Looks great, didn't take too close a look at the stepper component though, hopefully Justin Tim and Trevor can help pick up. gonna go grab breakfast

src/Input/Input.tsx

src/Input/InputWrapper.tsx

src/Input/Stepper.tsx

src/Input/Input.tsx

cheapsteak · 2019-08-03T15:53:03Z

src/Input/InputWrapper.tsx

+}: Props): ReturnType<React.FC> {
+  return (
+    <React.Fragment>
+      <label


We might want to have <label/> tag wrap the {label}, {description}, and {children} (not the error or info messages though), it would automatically associate the text we're using to describe the input with the input box for accessibility (more info). It'd also give focus to the input when clicking on the title, which is imo desirable

Alternatively, we could use aria-label or aria-labelledby, but those are more of a hassle (need to associate them by id, generating unique ids for association is a bit more of a hassle than associating via nesting hierarchy)

package.json

src/Input/Input.story.tsx

timbotnik · 2019-08-06T02:37:56Z

src/Input/Input.story.tsx

+        ...typography.base.base,
+        textTransform: "uppercase",
+        fontWeight: 600,
+        margin: 6,


Not sure where @adamatorres is on this, but 6 is not on the 4px grid. Considering this is a margin that will be applied on both sides of each axis, perhaps it's ok since the net effect is 12px 🤷‍♂

src/Input/Input.tsx

src/Input/InputWrapper.tsx

src/Input/Stepper.tsx

timbotnik · 2019-08-06T03:09:54Z

src/Input/Stepper.tsx

+    } else if (newValue !== value) {
+      setValue(newValue);
+    }
+    onChange(value);


I think this should be newValue?

We only want to call onChange with the value if it changes, and in this case newValue is the value the user has just tried to make the stepper. value willonly change if thats a valid value, so I call onChange(value) after all the logic determining if newValue is valid is done and value is set. Does that make sense?

src/Input/Stepper.tsx

adamatorres · 2019-08-08T15:14:12Z

Hey @mayakoneval -- Here's some design feedback on the inputs:

text size in the small input should be 13px, looks like it's 15px.
placeholder text color grey-light, it looks a bit darker than it should
disabled place holder text is too dark, it should be grey-lighter
Helper text needs to support an icon (specifically in error cases) and no icon. We currently don't have an example of basic helper text without an icon.
When helper text below an input has an icon and the text wraps, we shouldn't wrap the text under the icon.
Let's use the filled icons with the helper text.
I'm also seeing a delay of the focus state of the input after clicking into it, not sure what's going on there, but it should feel more instantaneous.

jgzuke · 2019-08-09T19:15:46Z

A few thoughts from using the new Inputs.

Its really awesome to have error states built in 🎉It'll make everything a lot more consistent and easier to build, I got to kill a bunch of code
Its a bit weird to have the input manage its own state instead of being a true managed/controlled component. It makes certain things impossible to use the new inputs with Ie recurly inputs (because recurly does dom manipulations), but it also means if the parent keeps the value in its own state as well we have extra render triggers.
Right now we pass the otherProps (classNames etc) to the inner input component which makes sense, however it means we cant pass any structure/position changing class names (w-full etc) since the input is wrapped. We can still position them by wrapping in divs or something, but it might make sense to take both props for the input, and for the outer wrapper. Or we could have the input live as a sibling to the label instead of inside, which might fix this problem

mayakoneval · 2019-08-09T23:06:12Z

A few thoughts from using the new Inputs.

Its really awesome to have error states built in 🎉It'll make everything a lot more consistent and easier to build, I got to kill a bunch of code

Its a bit weird to have the input manage its own state instead of being a true managed/controlled component. It makes certain things impossible to use the new inputs with Ie recurly inputs (because recurly does dom manipulations), but it also means if the parent keeps the value in its own state as well we have extra render triggers.

Right now we pass the otherProps (classNames etc) to the inner input component which makes sense, however it means we cant pass any structure/position changing class names (w-full etc) since the input is wrapped. We can still position them by wrapping in divs or something, but it might make sense to take both props for the input, and for the outer wrapper. Or we could have the input live as a sibling to the label instead of inside, which might fix this problem

I believe I fixed this with inputProps on the input and spreading the rest of the props on the outer div. Let me know if I'm off target :)

jgzuke · 2019-08-12T17:27:27Z

src/Input/InputWrapper.tsx

+    <div {...otherProps}>
+      <label
+        css={{
+          paddingBottom: 8,


Imo in a component library, a components box should be no larger than it needs to be, padding/margins should be set by the people using the components. However, this is a lot less annoying once https://github.com/mdg-private/engine-frontend/pull/1577 or something analogous lands so 🤷‍♂

@jgzuke From our space-kit meetings, it seems that all apollo products want the same spacing / font etc and that is why we decided to create a component library, to be able to use components that already have padding / margins etc and have builtin descriptions / error states. If you need to override this padding, either space-kit is wrong or the designs are not aligned! Let me know!

@mayakoneval the designs in https://app.zeplin.io/project/5d375cd742170577784b28ca/screen/5d41d60764e8c22bd5aeac20 have a smaller padding between them than two stacked inputs (with no labels)

This will stop us from getting errors in storybook while we're working. We don't need them, we have them in VS Code and CI

justinanastos · 2019-08-13T18:42:21Z

🚀 PR was released in v1.4.0 🚀

mayakoneval · 2019-08-13T23:30:58Z

package.json

@@ -13,6 +13,7 @@
    "clean:button": "rimraf Button",
    "clean:colors": "rimraf colors",
    "clean:fonts": "rimraf fonts",
+    "clean:formField": "rimraf FormField",


There is no FormField anymore, so this npm script won't do anything.

mayakoneval · 2019-08-13T23:32:20Z

CHANGELOG.md

@@ -61,6 +61,7 @@ _From #57_
 - Edit Modal prop descriptions, fix margin top on children [#41](https://github.com/apollographql/space-kit/pull/41)
 - Export constant from `colors` instead of requiring `import * as colors` (#35)
 - Add Card component (#34)
+- Text Inputs and Steppers [(#45)](https://github.com/apollographql/space-kit/pull/45)


This doesn't include Steppers, that file has been deleted

mayakoneval · 2019-08-13T23:33:27Z

src/TextField/TextField.tsx

+  /**
+   * Visible title
+   */
+  label: React.ReactNode;


I don't think this should be necessary. Credit card form, for example

mayakoneval force-pushed the mkoneval/text-inputs branch 2 times, most recently from 47adfea to 0f74a3e Compare August 3, 2019 00:38

mayakoneval requested review from justinanastos, cheapsteak, timbotnik and trevorblades August 3, 2019 00:52

cheapsteak reviewed Aug 3, 2019

View reviewed changes

mayakoneval force-pushed the mkoneval/text-inputs branch from 646a4e8 to 430910e Compare August 5, 2019 17:26

mayakoneval requested a review from adamatorres August 6, 2019 00:56

timbotnik suggested changes Aug 6, 2019

View reviewed changes

mayakoneval force-pushed the mkoneval/text-inputs branch 2 times, most recently from 3876d99 to 53b1bc8 Compare August 8, 2019 20:56

mayakoneval added the minor Increment the minor version when merged label Aug 8, 2019

mayakoneval requested review from timbotnik and cheapsteak August 9, 2019 23:04

jgzuke reviewed Aug 12, 2019

View reviewed changes

mayakoneval force-pushed the mkoneval/text-inputs branch from abe46a5 to 34a6717 Compare August 12, 2019 20:25

mayakoneval requested review from jgzuke and removed request for trevorblades and timbotnik August 12, 2019 20:26

mayakoneval force-pushed the mkoneval/text-inputs branch from 34a6717 to 69de1fa Compare August 12, 2019 20:29

Input story without input masking, stepper and inputs with icons

a0fdedc

mayakoneval force-pushed the mkoneval/text-inputs branch from 69de1fa to a0fdedc Compare August 12, 2019 20:32

justinanastos changed the title ~~AP-544 Text Inputs~~ AP-544 Implement Text Inputs Aug 13, 2019

justinanastos added 2 commits August 13, 2019 10:45

Make webpack only transpile for storybook

cb974b0

This will stop us from getting errors in storybook while we're working. We don't need them, we have them in VS Code and CI

Proposed changes to API

cb4d651

justinanastos merged commit 445a7c2 into master Aug 13, 2019

justinanastos added the released This issue/pull request has been released. label Aug 13, 2019

mayakoneval commented Aug 13, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AP-544 Implement Text Inputs #45

AP-544 Implement Text Inputs #45

mayakoneval commented Aug 3, 2019 •

edited

Loading

cheapsteak left a comment

cheapsteak Aug 3, 2019

timbotnik Aug 6, 2019

timbotnik Aug 6, 2019

mayakoneval Aug 7, 2019

adamatorres commented Aug 8, 2019 •

edited by mayakoneval

Loading

jgzuke commented Aug 9, 2019 •

edited

Loading

mayakoneval commented Aug 9, 2019

jgzuke Aug 12, 2019

mayakoneval Aug 12, 2019 •

edited

Loading

jgzuke Aug 13, 2019

justinanastos commented Aug 13, 2019

mayakoneval Aug 13, 2019

mayakoneval Aug 13, 2019

mayakoneval Aug 13, 2019

AP-544 Implement Text Inputs #45

AP-544 Implement Text Inputs #45

Conversation

mayakoneval commented Aug 3, 2019 • edited Loading

cheapsteak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamatorres commented Aug 8, 2019 • edited by mayakoneval Loading

jgzuke commented Aug 9, 2019 • edited Loading

mayakoneval commented Aug 9, 2019

Choose a reason for hiding this comment

mayakoneval Aug 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinanastos commented Aug 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayakoneval commented Aug 3, 2019 •

edited

Loading

adamatorres commented Aug 8, 2019 •

edited by mayakoneval

Loading

jgzuke commented Aug 9, 2019 •

edited

Loading

mayakoneval Aug 12, 2019 •

edited

Loading