Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 66 commits into from
May 13, 2017
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
66 commits
Select commit Hold shift + click to select a range
d90eef5
changes focusColor in FormLabel from palette.accent.A200 to palette.p…
peteratticusberg Apr 7, 2017
e1bf713
makes it so on focus the inkbar turns `palette.primary.A200` instead …
peteratticusberg Apr 7, 2017
ab6dfdd
changes checkbox 'checked' color to primary[500] from accent[500]
peteratticusberg Apr 7, 2017
a763d47
sets switch 'checked' color to primary[500] (used to be accent[500])
peteratticusberg Apr 7, 2017
ac69a5b
Merge branch 'next' of github.com:/callemall/material-ui into next
peteratticusberg Apr 7, 2017
050e3b1
modifies TextField to take `multiLine` and `rows` as props and forwar…
peteratticusberg Apr 8, 2017
0ed3174
adds styles to Input component for multiline inputs
peteratticusberg Apr 8, 2017
126e94e
adds a multiline input example
peteratticusberg Apr 8, 2017
9456d66
fixes linting errors
peteratticusberg Apr 8, 2017
9d18e60
Adds a test for the Input component
peteratticusberg Apr 8, 2017
9ad7d98
Adds a test for the TextField component
peteratticusberg Apr 8, 2017
e9ede40
adds visual regression tests for multiline text fields
peteratticusberg Apr 8, 2017
108c76f
fixes linting errors
peteratticusberg Apr 8, 2017
c3d1de3
Merge branch 'next' into multiline-textfield
peteratticusberg Apr 8, 2017
f0feccf
Adds defaultValue in as a prop to TextField to get defaultValue worki…
peteratticusberg Apr 9, 2017
8df8a6f
adds a defaultValue prop into Input to get defaultValues working cons…
peteratticusberg Apr 9, 2017
31268d3
adjusts the docs
peteratticusberg Apr 9, 2017
c17afe7
makes it so Input is responsible for deciding what element to render
peteratticusberg Apr 9, 2017
9d3635e
Makes it so that Input decides whether to render a textarea or an inp…
peteratticusberg Apr 9, 2017
38608e9
Makes it so the component prop will override the multiline default co…
peteratticusberg Apr 17, 2017
3f9e522
multiLine —> multiline
peteratticusberg Apr 17, 2017
ddd51d9
gets tests passing
peteratticusberg Apr 17, 2017
6bed551
removes default value from input
peteratticusberg Apr 17, 2017
94ac535
Merge branch 'next' of github.com:/callemall/material-ui into mtf
peteratticusberg Apr 17, 2017
b3b0534
fixes some typos
peteratticusberg Apr 17, 2017
3812485
adjusts syntax
peteratticusberg Apr 17, 2017
65e9441
Merge branch 'next' of github.com:/peteratticusberg/material-ui into …
peteratticusberg Apr 17, 2017
2cd3fa7
Apply the .underline class to the input wrapper instead of the input …
peteratticusberg Apr 17, 2017
31ed81a
gets visual regression tests Inputs/inputs and TextField/error passing
peteratticusberg Apr 17, 2017
99eddaf
adds new screenshot for multiline textfields
peteratticusberg Apr 17, 2017
2959331
fixes issue with inkbars for multiline textfields which was causing t…
peteratticusberg Apr 17, 2017
8d6b599
Merge branch 'next' into multiline-textfield
oliviertassinari Apr 18, 2017
fe7494f
Merge branch 'next' of github.com:/callemall/material-ui into next
peteratticusberg May 11, 2017
1f74023
Merge branch 'next' into mtf
peteratticusberg May 11, 2017
2a2577b
pulls in @mixby’s auto-resizing textarea
peteratticusberg May 11, 2017
6b681ed
renders an AutoResizingTextArea if multiline is specified but rows is…
peteratticusberg May 11, 2017
00c7e92
Adds a “Multiline Flexible” TextField demo and distinguishes it from …
peteratticusberg May 11, 2017
4bd4b41
swaps demo components
peteratticusberg May 11, 2017
dd21104
gets AutoResizingTextArea communicating with MuiFormControl
peteratticusberg May 11, 2017
bbdd7db
fixes small issue that was just introduced
peteratticusberg May 11, 2017
1f6a0f9
makes it so flow type annotations can be placed at the top of a file …
peteratticusberg May 11, 2017
7abc53b
Currently AutoResizingTextArea is the only “InputComponent” rendered …
peteratticusberg May 11, 2017
b49e7b3
slight adjustment to textfield demos
peteratticusberg May 11, 2017
eb536d8
removes unused classname
peteratticusberg May 11, 2017
5b6d156
a more idiomatic way of saying transparent
peteratticusberg May 11, 2017
b8195a2
having appearance: textfield set was causing a border box to appear a…
peteratticusberg May 11, 2017
65e1924
fixes some linting errors
peteratticusberg May 11, 2017
b5fee37
it’s now the wrapper that has the underline class
peteratticusberg May 11, 2017
d6a5ea1
Updates multiline tests for TextField
peteratticusberg May 11, 2017
5c66857
Adds tests for AutoResizingTextArea
peteratticusberg May 11, 2017
22e7650
cleans some things up…
peteratticusberg May 11, 2017
ab8a3bf
Replaces setTimeout hack with a slightly less offensive hack
peteratticusberg May 12, 2017
62a615f
Position the top of inkbar on top of the underline.
peteratticusberg May 12, 2017
98df3d6
remove TextFieldMultliLine test
peteratticusberg May 12, 2017
ed12ff3
adds TextFieldMultiline regression test back in with proper capitaliz…
peteratticusberg May 12, 2017
da71e70
Fix some comments
peteratticusberg May 12, 2017
0fe9bbc
new baseline for the TextFields demo
peteratticusberg May 12, 2017
25647aa
add a baseline test for the Multiline TextFields
peteratticusberg May 12, 2017
3161cca
empty commmit
peteratticusberg May 12, 2017
8f02f4e
Cleans up some comments and syntax
peteratticusberg May 13, 2017
a068813
Fixes positioning hack
peteratticusberg May 13, 2017
929e6c7
Remove unused methods from AutoResizingTextArea, debounce resize wind…
peteratticusberg May 13, 2017
3cde356
removes AutoResizingTextArea from project
peteratticusberg May 13, 2017
0b4509b
adds AutoResizingTextArea back in with different capitalization
peteratticusberg May 13, 2017
043d843
AutoResizingTextArea --> AutoResizingTextarea
peteratticusberg May 13, 2017
647d625
Add new regression tests for TextFields demo (the difference here was…
peteratticusberg May 13, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion docs/src/pages/component-demos/text-fields/TextFields.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,15 @@ export default class TextFields extends Component {
defaultValue="Hello World"
className={classes.input}
/>
<TextField
id="multiLine"
label="MultiLine"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiline?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

label="Multiline"

multiLine
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multiline

rows={3}
defaultValue="Default Value"
className={classes.input}
/>
</div>
);
}
}

25 changes: 24 additions & 1 deletion src/Input/Input.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,12 @@ export const styleSheet = createStyleSheet('MuiInput', (theme) => {
appearance: 'none',
},
},
multiLine: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multiline

resize: 'none',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to allow users to resize?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

multilinetext

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.

screen shot 2017-04-09 at 9 56 52 am

Copy link
Member

@oliviertassinari oliviertassinari Apr 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

'line-height': 'inherit',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for that style once you rebase the PR on next.

padding: '0px',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

padding: 0,

'margin-top': '12px',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

marginTop: 12,

},
disabled: {
color: theme.palette.text.disabled,
cursor: 'not-allowed',
Expand Down Expand Up @@ -97,6 +103,10 @@ export default class Input extends Component {
PropTypes.string,
PropTypes.func,
]),
/**
* The default value for the string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default value of the input element.

*/
defaultValue: PropTypes.string,
/**
* If `true`, the input will be disabled.
*/
Expand All @@ -113,6 +123,10 @@ export default class Input extends Component {
* The CSS class name of the input element.
*/
inputClassName: PropTypes.string,
/**
* If true, a textarea element will be rendered.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If `true`,

*/
multiLine: PropTypes.bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be called multiline? cc @mbrookes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, missed the cc. FWIW, I agree.

/**
* @ignore
*/
Expand All @@ -133,6 +147,10 @@ export default class Input extends Component {
* @ignore
*/
onFocus: PropTypes.func,
/**
* Number of rows to display when multiLine option is set to true.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multiline

*/
rows: PropTypes.number,
/**
* Type of the input element. It should be a valid HTML5 input type.
*/
Expand All @@ -152,6 +170,7 @@ export default class Input extends Component {
disabled: false,
type: 'text',
disableUnderline: false,
multiLine: false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multiline

};

static contextTypes = {
Expand Down Expand Up @@ -238,10 +257,12 @@ export default class Input extends Component {
const {
className: classNameProp,
component: ComponentProp,
inputClassName: inputClassNameProp,
defaultValue,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for that line.

disabled,
disableUnderline,
error: errorProp,
inputClassName: inputClassNameProp,
multiLine,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multiline

onBlur, // eslint-disable-line no-unused-vars
onFocus, // eslint-disable-line no-unused-vars
onChange, // eslint-disable-line no-unused-vars
Expand All @@ -265,6 +286,7 @@ export default class Input extends Component {
}, classNameProp);

const inputClassName = classNames(classes.input, {
[classes.multiLine]: multiLine,
[classes.underline]: !disableUnderline,
[classes.disabled]: disabled,
}, inputClassNameProp);
Expand All @@ -280,6 +302,7 @@ export default class Input extends Component {
onFocus={this.handleFocus}
onChange={this.handleChange}
disabled={disabled}
defaultValue={defaultValue}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirm, you don't need that line.

aria-required={required ? true : undefined}
{...other}
/>
Expand Down
6 changes: 6 additions & 0 deletions src/Input/Input.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ describe('<Input />', () => {
'should not have the area-required prop');
});

it('should render an <textarea /> inside the div when passed component="textarea"', () => {
const wrapper = shallow(<Input component="textarea" />);
const input = wrapper.find('textarea');
assert.strictEqual(input.is('textarea'), true, 'should be a <input>');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be a <textarea>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be '<textarea />'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just tried this but the test fails for input.is('<textarea />') 😞

});

it('should render a disabled <input />', () => {
const wrapper = shallow(<Input disabled />);
const input = wrapper.find('input');
Expand Down
23 changes: 21 additions & 2 deletions src/TextField/TextField.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ export default class TextField extends Component {
* The CSS class name of the root element.
*/
className: PropTypes.string,
/**
* The default value for the TextField
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default value of the `Input` element.

*/
defaultValue: PropTypes.string,
/**
* If `true`, the input will be disabled.
*/
Expand Down Expand Up @@ -45,6 +49,10 @@ export default class TextField extends Component {
* The CSS class name of the label element.
*/
labelClassName: PropTypes.string,
/**
* If true, a textarea element will be rendered.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If `true`,

*/
multiLine: PropTypes.bool,
/**
* Name attribute of the `Input` element.
*/
Expand All @@ -53,6 +61,10 @@ export default class TextField extends Component {
* If `true`, the label is displayed as required.
*/
required: PropTypes.bool,
/**
* Number of rows to display when multiLine option is set to true.
*/
rows: PropTypes.number,
/**
* Type attribute of the `Input` element. It should be a valid HTML5 input type.
*/
Expand Down Expand Up @@ -81,6 +93,7 @@ export default class TextField extends Component {
render() {
const {
className,
defaultValue,
disabled,
error,
inputClassName,
Expand All @@ -90,6 +103,8 @@ export default class TextField extends Component {
name,
required,
type,
multiLine,
rows,
value,
...other
} = this.props;
Expand All @@ -108,10 +123,14 @@ export default class TextField extends Component {
)}
<Input
className={inputClassName}
value={value}
component={multiLine ? 'textarea' : 'input'}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@peteratticusberg peteratticusberg Apr 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@oliviertassinari oliviertassinari Apr 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have two thoughs here:

  1. 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.
  2. Exposing a component property means user can override it. Hence, it should always take precedence, no matter the multiline state.

defaultValue={defaultValue}
disabled={disabled}
multiLine={multiLine}
name={name}
rows={rows}
type={type}
disabled={disabled}
value={value}
{...inputProps}
/>
</FormControl>
Expand Down
5 changes: 5 additions & 0 deletions src/TextField/TextField.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ describe('<TextField />', () => {
assert.strictEqual(wrapper.childAt(0).is('Input'), true);
});

it('should render an Input with component="textarea" when passed the multiLine prop', () => {
wrapper = shallow(<TextField multiLine />);
assert.strictEqual(wrapper.childAt(0).prop('component'), 'textarea');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the following more idiomatic, but do it as you prefer

.props().component

});

it('should pass inputClassName to the Input as className', () => {
wrapper.setProps({ inputClassName: 'foo' });
assert.strictEqual(wrapper.find('Input').hasClass('foo'), true);
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
17 changes: 17 additions & 0 deletions test/regressions/tests/TextField/TextFieldMultiLine.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// @flow weak

import React from 'react';
import TextField from 'material-ui/TextField';

export default function TextFieldError() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TextFieldMultiline

return (
<div>
<TextField
label="Foo"
multiLine
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multiline

rows={4}
value="Default text"
/>
</div>
);
}