[TextField] Initial implementation of optional underline

[newoga]

This resolves #2475. Related to and possibly fixes #2448 and #2233.

Done:

• Implemented a separate stateless text-field-underline.jsx component that defines its own style defaults, accepts disable, focus, and error state, and disabled, focus, and error style override configuration through props
• Simplified text-field.jsx to pass underlineStyle configurations to text-field-underline.jsx component

Todo:
- Create disabled underline example in docs Will do in a separate pull request since the TextField docs still need to be migrated to the new format.

Note:
The new text-field-underline.jsx currently takes the muiTheme through props from its parent text-field.jsx component. Stateless components also support context if we wanted to to make it work like the other components. Though for components like this that aren't meant to be alone, maybe we should consider accepting everything through props for simplicity. I'm open to discussing this though.

[newoga]

On a separate note:

If we like the idea of using a separate component for the text-field-underline, we can easily use the same pattern for helper/error, floating text, etc. Should make the code easier to reason about, esp with the inline styles.

[alitaheri]

maybe underlineVisible? underline feels like you need to pass the "Underline" node through this prop. @oliviertassinari Thoughts?

[newoga]

Yeah that's true, I can see that being confused for a prop that accept a component.

Naming things is hard 😸

[alitaheri]

Naming things is hard

Couldn't agree more >.>

[oliviertassinari]

what about underlineShow?

[alitaheri]

that's good too 😬

[newoga]

Let me know which one you want! I'm fine with either 😄

Now that I think about it, as a general rule, using a "positive-form" adjective might have a slight advantage over a verb/noun because some people specify boolean props as truthy without explicitly specifying the value of the prop as true.

For example some people and use eslint rules for this pattern:

/* CORRECT */
<Component
   active
/>

/* WRONG */
<MyComponent
   active={true}
/>

/* CORRECT */
<MyComponent
   active={false}
/>

[alitaheri]

go with underlineShow, easier to spell 😬

[alitaheri]

@oliviertassinari Do you think the generator can understand stateless functional components? (or whatever the hell facebook finally decides to call them)

[oliviertassinari]

Yes, it does (https://github.com/reactjs/react-docgen/blob/master/src/__tests__/main-test.js#L103)

[alitaheri]

nice 👍

[newoga]

Yeah I really like stateless functional components if you haven't realized 😅

I wanted to "test the waters" to see what you two thought, if you'd prefer me to write it createClass style, let me know. It's an easy switch.

[alitaheri]

@newoga I like them too, besides there can be heavily optimized by React, when they actually get around to it. I'm alright with this. @oliviertassinari ?should be migrate the stateless components to take advantage of these things?

[newoga]

If we go through with this, I'll throw out another style question/suggestion:

If we followed Airbnb's style guide, for example, we would define the propTypes and defaultProps as consts at the top of the file, then set them at the bottom.

So we would make this change to the order...:

import React from 'react';

const propTypes = {
};

const defaultProps = {
};

const TextFieldUnderline = (props) => {
  // impl
}

TextFieldUnderline.propTypes = propTypes;
TextFieldUnderline.defaultProps = defaultProps;

export default TextFieldUnderline;

Personally, I think I like this more so that when we add documentation to the props, they remain at the top of the file above the implementation instead of at the bottom...

[oliviertassinari]

I want to enforce react/sort-comp at some point. I completely agree 💯.

[oliviertassinari]

According to the react doc about the stateless component:

Functional components do not have lifecycle methods, but you can set .propTypes and .defaultProps as properties on the function.

This pattern is designed to encourage the creation of these simple components that should comprise large portions of your apps. In the future, we’ll also be able to make performance optimizations specific to these components by avoiding unnecessary checks and memory allocations.

Seems like as soon as we don't need a state, we should use this pattern 👍

[alitaheri]

@oliviertassinari Yeah, this is a great pattern. 👍 👍

[newoga]

Yay! 🎆

[newoga]

I think I'm pretty much done with this too. Let me know if you need anything else, otherwise I can squash and push.

Note: This pull request and #2473 has the same change to src/util/styles.js to add the merge() function from ImmutabilityHelper. So might make sense to merge one into master first, then update the other from master, unless there's some git magic I don't know about.

[alitaheri]

😱 😱 😱 You did a merge instead of rebase in your branch :D :D You're gonna hate git for the first time in your life when you try to squash this :D

I'll review right away

P.S. I had this issue before, I can help you if you get clueless.

[alitaheri]

@newoga I'm all good

@oliviertassinari Take a look too.

[oliviertassinari]

Looks fine so far, I'm gonna test it.

[neverfox]

@newoga @subjectix @oliviertassinari

Seems like as soon as we don't need a state, we should use this pattern

It is a great pattern but I'm very concerned about material-ui going in this direction while it's still not supported by babel-plugin-react-transform. Maybe this shouldn't be a concern because it wouldn't be likely that anyone would be changing the source on-the-fly? Discussed on #2493

[oliviertassinari]

What about considering the descriptions as a phrase and adding a dot at the end?

[alitaheri]

and use markdown in them too 😁

[oliviertassinari]

I think that we should create a TextField folder and move the text-field.jsx and the new text-field-underline.jsx inside this folder.
It's more common to see the file of component using uppercase (#1793).
Hence, I think that the file structure should be something like:

-TextField/
 -TextField.jsx
 -TextFieldUnderline.jsx
-text-field.jsx (with a simple reference to TextField/TextField.jsx)

[newoga]

@oliviertassinari I agree on the folder/new naming structure. Also, at some point in the future I think it'd be nice if the import matched the component name like mentioned in #1793. Would it make sense to export the component under two names and deprecate the access to the other one?
Maybe something like:

+-- TextField
|   +-- TextField.jsx
|   +-- TextFieldUnderline.jsx
|   +-- index.js -- this would default export TextField.jsx
text-field.jsx

Maybe we could make a util file like deprecatedExport.js:

import warning from 'warning';

function deprecatedExport(Component, deprecated, supported) {
  warning(false,
    `Importing ${Component.displayName} from '${deprecated}' has been deprecated, use '${supported}' instead.`);
  return Component;
}

export default deprecatedExport;

And then, like you suggested, we can rewrite text-field.jsx to export TextField.jsx from the new location and warn the developer:

import TextField from './TextField';
import deprecatedExport from './deprecatedExport';

export default deprecatedExport(TextField, 'material-ui/lib/text-field', 'material-ui/lib/TextField');

[alitaheri]

@newoga I like your idea 👍 👍

[newoga]

@oliviertassinari @subjectix If we like this deprecatedExport approach, should I submit that as a separate pull request that we can merge sooner?

[oliviertassinari]

@newoga I'm ok with this 👍.

[alitaheri]

@newoga Yeah that's a great idea 👍

[newoga]

Alright, I changed this set of components to the new PascalCase format in an appropriate TextField folder. I changed text-field.jsx to point to the new location, but did not add the deprecation warning (so it will just work).

I also made some documentation improvements like adding periods and markdown formatting such as code hint TextField.

@oliviertassinari, you had mentioned previously that you think the descriptions should be phrases with a period at the end. I added a period, but could you please give me an example description for one of the props? 😄 Just want to make sure I understand you correctly, I can fix the others afterwards.

[oliviertassinari]

@newoga The description is fine now. My point was simply to add a dot at the end.
@subjectix I'm ok with this PR. I guess that after a squash down of commit we can merge 👍.

[newoga]

Oh okay, sounds good. I'll squash now.

[alitaheri]

@newoga Thanks a lot. you're a huge help 👍 👍