[TextField] Add in support for multiline text fields #6553

peteratticusberg · 2017-04-08T23:01:01Z

Keeps the api consistent with the api from v0.x.

Are we good with this implementation? Happy to make changes if need to be.

…rimary.A200

…of `palette.accent.A200`

…d those along to `Input`

…ng consistently across inputs of type text and textareas

…istently for textareas and inputs of type text

oliviertassinari

That's a good start, thanks for working on it!

oliviertassinari · 2017-04-09T10:08:14Z

src/TextField/TextField.js

@@ -108,10 +123,14 @@ export default class TextField extends Component {
        )}
        <Input
          className={inputClassName}
-          value={value}
+          component={multiLine ? 'textarea' : 'input'}


I'm wondering, shouldn't it be the Input responsibility to render a textarea?

yea I think that's right. I'll change this.

When I started making this change I realized it didn't make as much sense as I thought :(

The <Input /> component takes a component prop, and it feels wrong to sometimes determine the rendered component via the component prop and other times determine the rendered component via the multiLine prop. We'd have to do something like this:

const InputComponent = multiLine ? 'textarea' : ComponentProp;

Which feels like unexpected behavior to me. Still happy to change this over if you like, but at this point I'm leaning towards keeping it the way it is.

I have two thoughs here:

The Input component is public and can be used on his own, hence all the multiline logic should be at the same place, in the Input.

Exposing a component property means user can override it. Hence, it should always take precedence, no matter the multiline state.

oliviertassinari · 2017-04-09T10:15:12Z

src/Input/Input.js

@@ -67,6 +67,12 @@ export const styleSheet = createStyleSheet('MuiInput', (theme) => {
        appearance: 'none',
      },
    },
+    multiLine: {
+      resize: 'none',


We don't want to allow users to resize?

I used v0 as the spec for this, which doesn't allow manual resizing for multiline text fields.

I should've used the official spec though...

Which says multiline text fields should work like this:

Note that the spec also talks about Text Areas which are different from multi-line text fields, and which I think we'll also want to implement in a separate PR.

You raise a good point but I think that the concern is different. Mine isn't about the spec but the UX. What do we win by disabling the resize feature?

If the users are allowed to resize the textbox it could result in unexpected things happening in the UI (the user could make it too wide, or too tall). Especially if we allow them to resize textareas horizontally, which I think would really be a break with convention.

We could allow users to resize textareas vertically but it would cause text to be cutoff mid-line unless we added javascript in to snap to (line-height * lines) sizes. Either solution here doesn't feel particularly clean to me.

My preference here would be to either a) not do this at all or b) handle this in a separate PR. Let me know though if you want me to add it in here and I can take care of it.

oliviertassinari · 2017-04-09T10:19:56Z

src/TextField/TextField.js

@@ -18,6 +18,10 @@ export default class TextField extends Component {
     */
    className: PropTypes.string,
    /**
+     * The default value for the TextField


Default value of the `Input` element.

oliviertassinari · 2017-04-09T10:21:54Z

src/Input/Input.js

@@ -114,6 +124,10 @@ export default class Input extends Component {
     */
    inputClassName: PropTypes.string,
    /**
+     * If true, a textarea element will be rendered.
+     */
+    multiLine: PropTypes.bool,


Should it be called multiline? cc @mbrookes

Yea, I actually had it as multiline originally but I noticed that in v0 it was multiLine so I refactored it to keep the API consistent. "multi-line" is one word, not two, so I think multiline would be more correct here.

I know v1 is already going to be introducing breaking changes, so maybe we just change it to multiline here?

Breaking changes aren't a concern. That new property name sounds better.

Sorry, missed the cc. FWIW, I agree.

oliviertassinari · 2017-04-09T10:22:40Z

src/Input/Input.js

@@ -98,6 +104,10 @@ export default class Input extends Component {
      PropTypes.func,
    ]),
    /**
+     * The default value for the string


Default value of the input element.

oliviertassinari · 2017-04-09T10:24:11Z

src/Input/Input.js

@@ -280,6 +302,7 @@ export default class Input extends Component {
          onFocus={this.handleFocus}
          onChange={this.handleChange}
          disabled={disabled}
+          defaultValue={defaultValue}


I don't think that we need to explicit the property, it's already inside the other variable when not destructured.

Yea, that's a good point. So everything was working find with regular text fields based on an <input /> but the defaultValue wasn't getting set for <textarea>'s until I explicitly passed defaultValue through as prop.

This probably isn't the best solution... Any ideas on how to improve this? Or why it might not have been working via the other object for textareas?

I confirm, you don't need that line.

peteratticusberg · 2017-04-09T14:05:01Z

of course, thanks for the comments, I'll push up some changes in a bit 👨‍💻

peteratticusberg · 2017-04-09T17:30:52Z

Hey, so after I merged in the latest version of next here I can no longer get my dev environment working. I get an issue with the command webpack --config webpack.dll.config.js:

ERROR in dll lib
Module not found: Error: Can't resolve 'babel-runtime' in '/Users/pete/code/mui/docs'
 @ dll lib

You'd think this would be easy to fix -- just install the babel-runtime dependency -- but the babel runtime dependency is already installed. I tried adding it to the dependencies of the docs project and still I got the same error. I'm running OSX Sierra, can't imagine that's affecting anything given this in a node error but who knows.

@oliviertassinari I know you added this recently, any ideas what the issue might be here? If not I can keep digging on this one.

peteratticusberg · 2017-04-09T17:58:34Z

Alright so here's the issue:

the babel runtime dependency doesn't specify a 'main' file in it's package.json and it also does not have an index.js file in it's root. The entry point is core-js.js. I was able to fix this by manually editing the package.json file in the babel-runtime dependency to include 'main': 'core-js.js'.

This is of course not the right solution to this problem.

I'm not sure what the right solution is but I'd imagine that it involves changing what we're doing in webpack.dll.config.js. I wonder if we could change the way we're dealing with dependencies there.

I'm also curious -- is it just me that's having this problem? Are you able to run yarn start on the latest version of next?

…ut based the multiLine prop

mbixby · 2017-05-11T22:45:49Z

@peteratticusberg any ideas about how to remove the setTimeout call in AutoResizingTextArea#handleChange? Seems hackish.

On my original branch I ran into issues with input height not being increased with newline on some slower devices (Safari on iPhone SE, complex app).

peteratticusberg · 2017-05-12T18:10:57Z

@mbixby yea that's a fair point. This removes it. I still don't feel great about this but I do think it's an improvement.

…ation

peteratticusberg · 2017-05-12T21:15:11Z

@oliviertassinari I recorded two new baselines, one for a Multiline TextField and another for the TextField demos page.

It looks like Argos does not recognize these new baselines and is still testing against the old ones. Have you seen this happen before? Any ideas how to fix it? yarn test:regressions passes locally

oliviertassinari · 2017-05-12T22:39:53Z

@peteratticusberg Thanks for pushing that branch forward! I'm gonna have a look at it tomorrow :). Regarding argos, that's expected. We shouldn't have to generate baselines in the first place, accepting changes is simpler (only members with write access can do so).

oliviertassinari

test/regressions/screenshots/chrome/TextField/TextFieldMultiLine.png seems to be a dead image.

oliviertassinari · 2017-05-13T09:35:35Z

test/regressions/tests/TextField/TextFieldMultiline.js

+
+export default function TextFieldMultiline() {
+  return (
+    <div>


I don't think that we need that extra div.

oliviertassinari · 2017-05-13T09:37:54Z

src/TextField/TextField.js

@@ -47,6 +51,10 @@ export default class TextField extends Component {
     */
    labelClassName: PropTypes.string,
    /**
+     * If `true`, a textarea element will be rendered.


may be we could add instead of an input.

oliviertassinari · 2017-05-13T09:38:48Z

src/Input/Input.js

+    multiline: {
+      resize: 'none',
+      padding: '0',
+      'margin-top': '12',


That's dead code, maybe you meant?

marginTop: 12,

oliviertassinari · 2017-05-13T09:39:11Z

src/Input/Input.js

+    },
+    multiline: {
+      resize: 'none',
+      padding: '0',


padding: 0,

oliviertassinari · 2017-05-13T09:40:33Z

src/Input/Input.js

+      padding: '0',
+      'margin-top': '12',
+    },
+    'multiline-wrapper': {


multilineWrapper: {

oliviertassinari · 2017-05-13T10:09:04Z

docs/src/pages/component-demos/text-fields/TextFields.js

+          label="MultiLine"
+          multiline
+          defaultValue="Default Value"
+          className={classes.input}


What about adding rowsMax={6}?

oliviertassinari · 2017-05-13T10:10:31Z

src/Input/AutoResizingTextArea.js

+    this.syncHeightWithShadow(undefined, event);
+  };
+
+  getInputNode() {


I don't think that we need that function indirection

yea I agree

oliviertassinari · 2017-05-13T10:11:11Z

src/Input/AutoResizingTextArea.js

+    return this.input;
+  }
+
+  setValue(value) {


I don't think that we should expose such method

oliviertassinari · 2017-05-13T10:15:04Z

src/Input/AutoResizingTextArea.js

+          rows={this.props.rows}
+          defaultValue={this.props.defaultValue}
+          readOnly
+          value={this.props.value}


I guess we should add aria-hidden="true".

oliviertassinari · 2017-05-13T10:15:13Z

src/Input/AutoResizingTextArea.js

+          tabIndex="-1"
+          rows={1}
+          readOnly
+          value={''}


I guess we should add aria-hidden="true".

peteratticusberg · 2017-05-13T13:25:44Z

Thanks for the review! Will fix this stuff up.

And that's weird, TextFieldMultiline.png should be present: https://github.com/peteratticusberg/material-ui/blob/3161ccab80b454250b3288eb7eadd4a42e850757/test/regressions/screenshots/chrome/regression-TextField/TextFieldMultiline.png

…ow event

… that we changed the label from MultiLine --> Multiline)

peteratticusberg · 2017-05-13T14:08:17Z

Alright! Addressed all those comments. Had to push up a new baseline for the TextField demo page (the difference was changing the label from MultiLine --> Multiline) don't know if we need to let argo know about that.

Let me know if there's anything else you think we need to address before we merge this in.

oliviertassinari

Let's go for it, I have seen some small issue I'm gonna take care of. Thanks.

peteratticusberg · 2017-05-13T17:19:17Z

Awesome. Thanks for the help with this! 🎊

peteratticusberg added 14 commits April 7, 2017 15:33

changes focusColor in FormLabel from palette.accent.A200 to palette.p…

d90eef5

…rimary.A200

makes it so on focus the inkbar turns palette.primary.A200 instead …

e1bf713

…of `palette.accent.A200`

changes checkbox 'checked' color to primary[500] from accent[500]

ab6dfdd

sets switch 'checked' color to primary[500] (used to be accent[500])

a763d47

Merge branch 'next' of github.com:/callemall/material-ui into next

ac69a5b

modifies TextField to take multiLine and rows as props and forwar…

050e3b1

…d those along to `Input`

adds styles to Input component for multiline inputs

0ed3174

adds a multiline input example

126e94e

fixes linting errors

9456d66

Adds a test for the Input component

9d18e60

Adds a test for the TextField component

9ad7d98

adds visual regression tests for multiline text fields

e9ede40

fixes linting errors

108c76f

Merge branch 'next' into multiline-textfield

c3d1de3

muibot added the PR: Needs Review label Apr 8, 2017

peteratticusberg changed the title ~~Adds in support for multiline text fields~~ [TextField] Adds in support for multiline text fields (v1.0.0) Apr 8, 2017

peteratticusberg added 3 commits April 8, 2017 21:13

Adds defaultValue in as a prop to TextField to get defaultValue worki…

f0feccf

…ng consistently across inputs of type text and textareas

adds a defaultValue prop into Input to get defaultValues working cons…

8df8a6f

…istently for textareas and inputs of type text

adjusts the docs

31268d3

oliviertassinari requested changes Apr 9, 2017

View reviewed changes

oliviertassinari added next PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Apr 9, 2017

oliviertassinari changed the title ~~[TextField] Adds in support for multiline text fields (v1.0.0)~~ [TextField] Add in support for multiline text fields Apr 9, 2017

oliviertassinari added the component: text field This is the name of the generic UI component, not the React module! label Apr 9, 2017

peteratticusberg added 2 commits April 9, 2017 14:57

makes it so Input is responsible for deciding what element to render

c17afe7

Makes it so that Input decides whether to render a textarea or an inp…

9d3635e

…ut based the multiLine prop

Replaces setTimeout hack with a slightly less offensive hack

ab8a3bf

peteratticusberg added 7 commits May 12, 2017 14:43

Position the top of inkbar on top of the underline.

62a615f

remove TextFieldMultliLine test

98df3d6

adds TextFieldMultiline regression test back in with proper capitaliz…

ed12ff3

…ation

Fix some comments

da71e70

new baseline for the TextFields demo

0fe9bbc

add a baseline test for the Multiline TextFields

25647aa

empty commmit

3161cca

oliviertassinari requested changes May 13, 2017

View reviewed changes

peteratticusberg added 7 commits May 13, 2017 09:35

Cleans up some comments and syntax

8f02f4e

Fixes positioning hack

a068813

Remove unused methods from AutoResizingTextArea, debounce resize wind…

929e6c7

…ow event

removes AutoResizingTextArea from project

3cde356

adds AutoResizingTextArea back in with different capitalization

0b4509b

AutoResizingTextArea --> AutoResizingTextarea

043d843

Add new regression tests for TextFields demo (the difference here was…

647d625

… that we changed the label from MultiLine --> Multiline)

oliviertassinari approved these changes May 13, 2017

View reviewed changes

oliviertassinari added PR: Review Accepted and removed PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels May 13, 2017

oliviertassinari merged commit d6685e8 into mui:next May 13, 2017

muibot added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label May 13, 2017

oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label May 13, 2017

[TextField] Add in support for multiline text fields #6553

[TextField] Add in support for multiline text fields #6553

Conversation

peteratticusberg commented Apr 8, 2017

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peteratticusberg Apr 9, 2017 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Apr 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Apr 9, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

peteratticusberg commented Apr 9, 2017

peteratticusberg commented Apr 9, 2017

peteratticusberg commented Apr 9, 2017

mbixby commented May 11, 2017

peteratticusberg commented May 12, 2017

peteratticusberg commented May 12, 2017

oliviertassinari commented May 12, 2017

oliviertassinari 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

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

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

peteratticusberg commented May 13, 2017

peteratticusberg commented May 13, 2017

oliviertassinari left a comment

Choose a reason for hiding this comment

peteratticusberg commented May 13, 2017 • edited Loading

peteratticusberg Apr 9, 2017 •

edited

Loading

oliviertassinari Apr 9, 2017 •

edited

Loading

oliviertassinari Apr 9, 2017 •

edited

Loading

peteratticusberg commented May 13, 2017 •

edited

Loading