[TextField] Make it meet guidelines #6566

kybarg · 2017-04-10T13:12:01Z

SSR floating label render closes [TextField] Labels aren't floating on first SSR render #6425
~~Multiline text support closes [TextField] Multiline text support #6288~~ [TextField] Add in support for multiline text fields #6553
Implement helper text closes [TextField] Implement helper text #2474
Implement states as for specs
Remove unused TextFieldLabel component closes [TextFieldLabel] Is there need for this component #6580
~~Update "Supported components" docs page~~

kybarg · 2017-04-10T13:26:06Z

src/Form/FormControl.js

+    const dirty = props.children.some((child) => {
+      return child.type && child.type.name === 'Input' &&
+        child.props.value && child.props.value.length > 0;
+    });


This looks complex. Any better way?

Good question! I can't think of a better alternative 🤔 .

oliviertassinari · 2017-04-10T16:02:11Z

Notice that there is an ongoing effort for the multiline implementation #6553.

kybarg · 2017-04-10T19:49:28Z

@oliviertassinari that's great. Will take it into account.

oliviertassinari · 2017-04-11T19:42:39Z

src/Form/FormControl.js

+  constructor(props, context) {
+    super(props, context);
+
+    const dirty = props.children.some((child) => {


Nothing guarantee the props.children to be a flat array.

React provides a Children.map utility function.

oliviertassinari · 2017-04-11T19:43:13Z

src/Form/FormControl.js

+
+    const dirty = props.children.some((child) => {
+      return child.type && child.type.name === 'Input' &&
+        child.props.value && child.props.value.length > 0;


What about importing the isDirty function from the Input.js? I think that's safer that way.

oliviertassinari · 2017-04-11T21:15:08Z

src/Form/FormControl.js

+    const dirty = props.children.some((child) => {
+      return child.type && child.type.name === 'Input' &&
+        child.props.value && child.props.value.length > 0;
+    });


Good question! I can't think of a better alternative 🤔 .

oliviertassinari

Looks like it's going in the right direction 😄 .

oliviertassinari · 2017-04-14T11:57:56Z

src/Form/FormControl.js

@@ -78,6 +79,18 @@ export default class FormControl extends Component {
    };
  }

+  componentWillMount() {
+    let dirty = false;
+    Children.map(this.props.children, (child) => {


What about a forEach?

Yes this should work in this case)

oliviertassinari · 2017-04-14T11:58:37Z

src/Form/FormHelperText.js

+    root: {
+      color: theme.palette.input.helperText,
+      fontSize: 12,
+      lineHeight: 1.33333333,


I don't think that we need as many significant number.

@oliviertassinari In Chrome 1.3333333 lineheight = 16px height, 1.333333 = 14 px 😄 I set it 1.4 also gives 16px but don't know how in can affect browsers when using retina display.

oliviertassinari · 2017-04-14T12:05:50Z

src/TextField/TextFieldLabel.js

-import customPropTypes from '../utils/customPropTypes';
-import { FormLabel } from '../Form';
-
-export const styleSheet = createStyleSheet('MuiTextFieldLabel', (theme) => {


Oops, a dead component 🙈 , good catch!

…text-field-guidelines

mbrookes · 2017-04-15T15:02:45Z

src/Form/FormHelperText.js

@@ -12,7 +12,7 @@ export const styleSheet = createStyleSheet('MuiFormHelperText', (theme) => {
    root: {
      color: theme.palette.input.helperText,
      fontSize: 12,
-      lineHeight: 1.33333333,
+      lineHeight: 1.4,


Wouldn't 1.3' round to 1.3?

1.3 results to 15px line-height in webkit...

oliviertassinari · 2017-05-29T20:02:23Z

@kybarg The InputLabel extends the FormLabel, it's specifically designed for the input context.

I have one concern with the new height of the TextField.
While the height is incorrect on the current next branch, I think that we have the occasion to make it right: 62px with a label as correctly implemented by Polymer: https://www.webcomponents.org/element/PolymerElements/paper-input

And maybe 42px without the label or maybe not, I don't know:

What do you think? Increasing the height when a helper text is used is a good idea 👍 .

oliviertassinari · 2017-05-29T20:03:59Z

src/Input/InputLabel.js

  },
  shrink: {
-    transform: 'translate(0, 0px) scale(0.75)',
+    transform: `translate(0, ${theme.spacing.unit * 2 + 2.5}px) scale(0.75)`,


I would avoid any 1/x px computation as unpredictable and subject to GPU floating error.

But why not keeping it for now.

Without this hack, other inputs labels flicker when focusing one. Couldn't find better solution.
Only if we switch from transitions to animation.

kybarg · 2017-05-29T20:13:30Z

@oliviertassinari I wouldn't stick with Polymer's implementation. Found many differences between it and official guidelines. material-components-web is better example but I think it is still too raw to follow.
I think you are right with 42px variant. I wanted default field to be 48px high. But it turns that 48px high field is different implementation (full-width field).

Placeholder styles look ugly. laceholder prefixing is to be impplemented in next JSS versions.

oliviertassinari

material-web-components and polymer are two excellent reference implementation. Pick one and we will be happy 😄 .

oliviertassinari · 2017-05-29T20:54:28Z

src/Input/Input.js

+          easing: theme.transitions.easing.ease,
+        }),
+      },
+      '&:focus::-webkit-input-placeholder': {


Can't we use a single selector instead of repeating the opacity: property?

@oliviertassinari JSS currently doesn't support prefixing ::placeholder selector. But there is discussion on this issue. Hope the will release it soon.

I was suggesting doing the following, but I'm not 100% sure it's working.

'&:focus::-moz-placeholder, &:focus:-ms-input-placeholder, &:focus::-moz-placeholder, &:focus::-webkit-input-placeholder': { opacity: theme.palette.type === 'light' ? 0.42 : 0.5, },

@oliviertassinari unfortunately not 😢

…text-field-guidelines # Conflicts: # src/styles/MuiThemeProvider.js

kybarg · 2017-05-30T10:57:45Z

@oliviertassinari I've made this based on sticker sheet and specs

mbrookes

@kybarg So nearly there - just some tiny changes to standardize the prop labels, but before you follow my comments, when I got to the bottom there was this one:

@oliviertassinari Any preference?

mbrookes · 2017-05-30T13:50:12Z

src/Form/FormControl.js

@@ -115,6 +127,10 @@ FormControl.propTypes = {
   */
  className: PropTypes.string,
  /**
+   * If `true`, the label should be displayed in an disabled state.
+   */


'a disabled state'

mbrookes · 2017-05-30T13:52:19Z

src/Form/FormHelperText.js

+  disabled: PropTypes.bool,
+  /**
+   * Whether the helper text should be displayed in an error state.
+   */


'If true,'

mbrookes · 2017-05-30T13:52:21Z

src/Form/FormHelperText.js

+   */
+  className: PropTypes.string,
+  /**
+   * Whether the helper text should be displayed in an disabled state.


'If true,' ... 'a disabled state.'

mbrookes · 2017-05-30T13:53:34Z

src/Form/FormHelperText.spec.js

+  });
+
+  describe('prop: error', () => {
+    it('should show an error class', () => {


'should have an error class' / 'should set the error class'?

mbrookes · 2017-05-30T13:55:06Z

src/Form/FormLabel.js

@@ -91,6 +100,10 @@ FormLabel.propTypes = {
   */
  className: PropTypes.string,
  /**
+   * Whether the label should be displayed in an disabled state.


'If true,' ... 'a disabled state.'

mbrookes · 2017-05-30T13:55:29Z

src/Form/FormLabel.js

+   * Whether the label should be displayed in an disabled state.
+   */
+  disabled: PropTypes.bool,
+  /**
   * Whether the label should be displayed in an error state.


mbrookes · 2017-05-30T13:57:51Z

src/Input/InputLabel.js

@@ -81,6 +86,10 @@ InputLabel.propTypes = {
   */
  disableAnimation: PropTypes.bool,
  /**
+   * If `true`, apply disabled class.


'If true, apply the disabled CSS class'.

Hmmm, now I wonder if all the others should follow that pattern. It's clearer to the user what the outcome of applying this prop is when it comes to custom styling.

oliviertassinari · 2017-05-30T18:58:58Z

@kybarg You rock, let's merge this 🚀 !

MichaelMure · 2017-06-01T14:00:13Z

Hello

After this merge, the height of a TextField is much bigger than previously, with a big margin on top. I didn't find a way to make it more compact. Is there one ?

Thank you for your work :)

kybarg · 2017-06-01T15:06:32Z

@micaste Hi! Default input has height of 48px, with floating label 72px. This is what proposed by specs. But you can adjust it with styles.

oliviertassinari · 2017-06-01T15:28:58Z

@kybarg I'm wondering if we shouldn't reduce the height of the TextField and expose an optional property to enable some margin, the margins suggested by the documentation.

kybarg · 2017-06-01T21:27:30Z

@oliviertassinari I see spec as priority as it tries to cover most common cases. I believe non standard implementation is not very common 💭 Also I want to add "dense" variant. Maybe this can help in some cases as well 😄

MichaelMure · 2017-06-01T23:07:54Z

@kybarg any style override to suggest ? I tried but I can only break the thing apparently.

kybarg · 2017-06-02T00:55:36Z

@MichaelMure chat here https://gitter.im/callemall/material-ui

Adjusted styles accordingto specs, fixed mui#6425

7f966d0

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

kybarg changed the title ~~[TextField] Make it meet guidelines~~ [TextField] Make it meet guidelines (next) Apr 10, 2017

kybarg changed the title ~~[TextField] Make it meet guidelines (next)~~ [TextField] Make it meet guidelines Apr 10, 2017

muibot added 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 10, 2017

kybarg commented Apr 10, 2017

View reviewed changes

kybarg mentioned this pull request Apr 10, 2017

[TextField] Mask #2130

Closed

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

danhardman mentioned this pull request Apr 11, 2017

[TextField] Add icon prop to Text Field #2932

Closed

oliviertassinari requested changes Apr 11, 2017

View reviewed changes

kybarg added 2 commits April 12, 2017 02:11

Merge branch 'callemall/next' into next-text-field-guidelines

10a5c0f

Fixes, simplified styles

6e8ad06

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

kybarg added 8 commits April 12, 2017 02:52

Removed dead TextFieldLabel component as for mui#6580

b5fb82a

Added colors according to specs, InputLabel disabled prop

6cece85

Avoiding label flicker and blurry text in webkit browsers

7b7fa13

More smooth label animation.

4716659

More smooth font-size animation

6f83e5b

Proper error color, added FormHelperText component

186a251

FormHelperText docs

7abcec9

Fixed lint errors

d02154c

oliviertassinari reviewed Apr 14, 2017

View reviewed changes

oliviertassinari removed the PR: Needs Review label Apr 14, 2017

Added new component to styles order, fixes

dffc976

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

Merge remote-tracking branch 'refs/remotes/callemall/next' into next-…

890217f

…text-field-guidelines

mbrookes reviewed Apr 15, 2017

View reviewed changes

oliviertassinari reviewed May 29, 2017

View reviewed changes

kybarg added 2 commits May 29, 2017 23:35

Fixes

f30dbea

Placeholder styles look ugly. laceholder prefixing is to be impplemented in next JSS versions.

Fixed lint errors

0419922

oliviertassinari reviewed May 29, 2017

View reviewed changes

Merge remote-tracking branch 'refs/remotes/callemall/next' into next-…

c37fe6e

…text-field-guidelines # Conflicts: # src/styles/MuiThemeProvider.js

oliviertassinari approved these changes May 30, 2017

View reviewed changes

oliviertassinari added PR: Review Accepted and removed PR: out-of-date The pull request has merge conflicts and can't be merged 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 30, 2017

mbrookes requested changes May 30, 2017

View reviewed changes

mbrookes 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 30, 2017

kybarg added 2 commits May 30, 2017 17:21

Typo fixes

30be343

Typo fixes 2

60ae4d7

oliviertassinari merged commit 1c96695 into mui:next May 30, 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 30, 2017

kybarg deleted the next-text-field-guidelines branch May 30, 2017 20:08

oliviertassinari mentioned this pull request May 30, 2017

[TextField] Fix broken isDirty logic #7008

Merged

3 tasks

kybarg mentioned this pull request May 30, 2017

[TextField] label is hidden by yellow-autofill-box in chrome #7003

Closed

oliviertassinari mentioned this pull request Jun 12, 2017

[TextField]: add a compact property that reduce height #7109

Closed

3 tasks

oliviertassinari mentioned this pull request Jun 28, 2017

Textfield in next is not like material #7285

Closed

[TextField] Make it meet guidelines #6566

[TextField] Make it meet guidelines #6566

Conversation

kybarg commented Apr 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Apr 10, 2017 • edited

kybarg commented Apr 10, 2017

Choose a reason for hiding this comment

oliviertassinari Apr 11, 2017 • edited

Choose a reason for hiding this comment

oliviertassinari Apr 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

oliviertassinari commented May 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kybarg commented May 29, 2017 • edited

oliviertassinari left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari May 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kybarg commented May 30, 2017 • edited

mbrookes left a comment • edited

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

oliviertassinari commented May 30, 2017 • edited

MichaelMure commented Jun 1, 2017

kybarg commented Jun 1, 2017

oliviertassinari commented Jun 1, 2017 • edited

kybarg commented Jun 1, 2017

MichaelMure commented Jun 1, 2017

kybarg commented Jun 2, 2017 • edited by oliviertassinari

kybarg commented Apr 10, 2017 •

edited

oliviertassinari commented Apr 10, 2017 •

edited

oliviertassinari Apr 11, 2017 •

edited

oliviertassinari Apr 11, 2017 •

edited

oliviertassinari commented May 29, 2017 •

edited

kybarg commented May 29, 2017 •

edited

oliviertassinari left a comment •

edited

oliviertassinari May 29, 2017 •

edited

kybarg commented May 30, 2017 •

edited

mbrookes left a comment •

edited

oliviertassinari commented May 30, 2017 •

edited

oliviertassinari commented Jun 1, 2017 •

edited

kybarg commented Jun 2, 2017 •

edited by oliviertassinari