Multiline Input and consistent alignment #4270
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chromakode
force-pushed
the
mgood/DESKTOP-1837
branch
from
17d1721 to
9fb8002
Compare
Also clean up alignment of floating labels / hints / text.
Due to the internals of the material-ui TextField, it is difficult for us to expand the height of an input field to contain the hint. As a quick and dirty workaround, let's compromise by clipping the hint to the size of the input.
chromakode
force-pushed
the
mgood/DESKTOP-1837
branch
from
9fb8002 to
450469c
Compare
MarcoPolo
reviewed
Sep 14, 2016
| @@ -6,7 +6,7 @@ export type SmallInputProps = { | |||
| hintText: string, | |||
| label: string, | |||
| value: ?string, | |||
| onChange: (next: string) => void, | |||
| onChange?: (next: string) => void, | |||
There was a problem hiding this comment.
Why is this optional now?
There was a problem hiding this comment.
I added this since <Input>'s onChange is similarly optional. I noticed this because I didn't specify handlers in the dumb sheet. It looks like in general we don't require event handlers in the other events of the <Input> flowtype.
|
👍 |
MarcoPolo
suggested changes
Sep 14, 2016
| @@ -6,7 +6,7 @@ export type SmallInputProps = { | |||
| hintText: string, | |||
| label: string, | |||
| value: ?string, | |||
| onChange: (next: string) => void, | |||
| onChange?: (next: string) => void, | |||
MarcoPolo
approved these changes
Sep 14, 2016
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This branch adds a bunch of fiddly tweaks to the
<Input>element to enable it to serve as a multi-line input, allowing us to remove<MultiLineInput>, which is currently a separate code path. The styling of the hint position, spacing between underline and input text, and floating text position have been tweaked to better match the style guide as well as each other in the different combinations of multiline / floating label / hint. Special attention was paid to lining up the gray placeholder text baseline with the black value text.It's a big improvement, but it isn't perfect -- in the interest of time, this is a 90% effort. We're kind of pushing the limits of what's convenient to style in the Material-UI
<TextField>. The next step would be to write our own component, but I've tried hard not to resort to that here, merely taking out some of the hacks and fiddling with the styling to get it mostly right. In the future pass we should probably write a component that serves our needs precisely.One compromise I had to make to merge the inputs was dropping support for expanding inputs to fit the size of their hint text. Previously, the paper key inputs would expand to contain a multi-line hint with the full length of a paper key. Now the hint gets constrained to the space of the single-line empty input. I've discussed this change w/ design and it's ok to move forward with.
I've checked the impact of these changes by running local visdiffs during development and resolved the problems I could find.
This changeset will benefit from the Lato fonts update which will resolve some text measurement glitchiness.
👓 @keybase/react-hackers