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] Implement helper text #2474

Closed
newoga opened this issue Dec 11, 2015 · 11 comments · Fixed by #6566
Closed

[TextField] Implement helper text #2474

newoga opened this issue Dec 11, 2015 · 11 comments · Fixed by #6566
Labels
component: text field This is the name of the generic UI component, not the React module! design: material This is about Material Design, please involve a visual or UX designer in the process new feature New feature or request

Comments

@newoga
Copy link
Contributor

newoga commented Dec 11, 2015

In the current implementation of TextField, the space where the error text resides should also be able to support helper text.

According the the spec:

Helper text, or hint text, should appear below the text field and either be persistently visible, or only visible on focus.

IMO: I don't like how how the spec says, "or hint text," because the term hint text is used elsewhere in the spec as it relates to placeholder text.

Nevertheless, we should consider making a child HelperText component for TextField that supports changing color based on error state.

helper_text.png

@newoga
Copy link
Contributor Author

newoga commented Dec 11, 2015

I suppose there are a couple of different ways to implement this, but how to implement this properly while maintaining backwards compatibility will be interesting:

Assuming default themes (like images above), I suppose these are our approaches:

Approach (1): Deprecate errorText in favor of helperText, and manage error state through error prop

  1. Add prop error that provides error state to the component
  2. Deprecate errorText and errorStyle props (add a warning when used) but if errorText is provided, then ignore error and continue operating with old behavior
  3. If errorText and helperText are used together, provide a separate warning, but ignore errorText implementation and use helperText instead
/* Example 1.Render helper text as red */
<TextField error={true} helperText="This is helper text" />

/* Example 2. Render helper text as grey */
<TextField error={false} helperText="This is helper text" />

/* Example 3. Provide deprecation warning, ignore `error` prop,
and render the helper text as red for backwards compatibility */
<TextField error={true || false} errorText="This is error text" />

/* Example 4. Provide deprecation warning, but behave like example 1 or 2 */
<TextField
  error={true || false} 
  helperText="This is helper text"
  errorText="This is error text" />

Approach (2): Support errorText and helperText, and manage error state through error prop

This is same as approach (1), but we continue supporting both, perhaps like below:

/* Example 1.Render helper text as red */
<TextField error={true} helperText="This is helper text" />

/* Example 2. Render helper text as grey */
<TextField error={false} helperText="This is helper text" />

/* Example 3. Render the error text as red */
<TextField error={true} errorText="This is error text" />

/* Example 3. Render nothing */
<TextField error={false} errorText="This is error text" />

/* Example 4. No idea... For backwards compatibility, I suppose the `error` prop would
have to be assumed true in this case which is weird */
<TextField errorText="This is error text" />

/* Example 5. Error text will show as red */
<TextField
  error={true} 
  helperText="This is helper text"
  errorText="This is error text" />

/* Example 6. Helper text will show as grey */
<TextField
  error={false} 
  helperText="This is helper text"
  errorText="This is error text" />

/* Example 6. I suppose helperText will show as grey, and `error` prop would be
assumed false */
<TextField
  helperText="This is helper text"
  errorText="This is error text" />

Overall, I think approach 2 can get really weird and might confuse users in the long run. But it does have an interesting ability to be able to control whether helper or error text shows based on error state. For example, your error text might change based on validation rules, but after they clear, the original helper text will show again without you have to reset it.

@newoga
Copy link
Contributor Author

newoga commented Dec 11, 2015

Just remembered that this has implications on the underline as well, since we're currently using the presence of errorText to manage error state. Though maybe that means we can do this:

Approach (3): Support errorText and helperText, show helperText when errorText is not defined

/* Example 1. Show helper text as grey */
<TextField
  helperText="This is helper text" />

/* Example 2. Show error text as red */
<TextField
  helperText="This is helper text"
  errorText="This is error text" />

/* Example 3. Maybe we could optionally support something like this?
If errorText is true, show the helperText as red. 
I just don't like the concept of errorText possibly being a boolean */
<TextField
  helperText="This is helper text"
  errorText={true} />

@alitaheri alitaheri added new feature New feature or request design: material This is about Material Design, please involve a visual or UX designer in the process labels Dec 11, 2015
@alitaheri
Copy link
Member

@newoga I think the first approach is the best approach. There are some other concepts you're missing. styles, and customization. I think if we have both helperText and errorText, then what would the style look like? errorTextStyle? or helperTextStyle? the other 2 approaches can be really confusing, but the first one is very explicit, (this is valid? if not tell me why through the helperText). I like this proposal in general 👍 @oliviertassinari What do you think?

@newoga
Copy link
Contributor Author

newoga commented Dec 11, 2015

Yeah you're right about the overriding styles bit. I did think a little bit about what the implications would be, but my comment was already getting excessively long, sorry about that!

I agree, I think I like the first approach the most too. And I like having a smaller API surface area :) Plus, if a user really wanted approaches 2 or 3, it'd probably be simple enough for them to create it by wrapping the component in approach 1.

I'm open to debate though!

@alitaheri
Copy link
Member

@newoga I think the first approach is the right way to do it. and come on, if they wanted to implement approach 2 or 3, it's just a ternary operator :D
I'd like to hear @oliviertassinari's insight on this too.

@Taakn
Copy link

Taakn commented Jun 2, 2016

Any news on this ? This would make forms a lot more readable. Thanks !

@avocadowastaken
Copy link
Contributor

@newoga I think that the third approach is easier to maintain, like:

<TextField helperText="Helping Tip." errorText={someNullableErrorVal} >

So while our error var is null or empty - helper text will be shown
And don't need to mix things like that

<TextField error={!!someNullableErrorVal} helperText={someNullableErrorVal || 'Helping Tip.'} />

@oliviertassinari oliviertassinari added component: text field This is the name of the generic UI component, not the React module! and removed component: text field This is the name of the generic UI component, not the React module! labels Oct 19, 2016
@ce07c3
Copy link

ce07c3 commented Jan 5, 2017

Any update on this one? If we can agree on an approach, perhaps it'd be easier to develop.

@newoga
Copy link
Contributor Author

newoga commented Jan 5, 2017

I thought of these approaches as it related to the current TextField component design and tried to make them backwards compatible. The TextField component design in the next branch is improved, albeit is a breaking change. I would have to revisit the code but I imagine implementing helper text in a composed way should be much simpler in that branch.

I probably would recommend not implementing any of these approaches on the old API and just focus on making sure we can implement helper text on the new one.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 5, 2017

That's definitely a missing feature in the next branch. It would be good to agree on the API so anyone could give a shot at implementing it. I think that we gonna need two level of abstraction. One new component for the low-level API and one new property to the TextField for the high-level API.

@newoga
Copy link
Contributor Author

newoga commented Jan 5, 2017

@oliviertassinari Sounds good. I need to and will try to get caught up on the current design of things in the next branch so I can provide some better feedback. Also, now that we have a initial versions of Tabs and Toolbar merged, I'd like to deliver on some AppBar promises that I've made over a year ago 😅.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: text field This is the name of the generic UI component, not the React module! design: material This is about Material Design, please involve a visual or UX designer in the process new feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants