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] Multiline does not carry 'color' style property. #3059 #3099

Closed
wants to merge 2 commits into from

Conversation

third
Copy link

@third third commented Jan 30, 2016

Added update to propagate inputStyle to textarea.

(First-time contributor here)

Third Santor added 2 commits January 30, 2016 00:16
…i#3059

inputStyle is propagated to single-line TextField but not when on multi-line mode (textarea).
@newoga
Copy link
Contributor

newoga commented Feb 1, 2016

@third Thanks much for your (first-time) contribution. Welcome to the community, it's super appreciated. I think the textareaStyles weren't designed to merge in the style the way you did it. The style prop is usually used exclusively for the root element. That being said, what you're trying to accomplish isn't currently supported so we can still investigate this more.

@oliviertassinari @alitaheri @mbrookes
Is there a reason why we're defining textarea styles in here in TextField.jsx rather than in the enhanced-textarea.js component?

If not, I recommend we change TextField.jsx to not define any of the textarea styles and defined them all in EnhancedTextArea. After that, we may want to consider passing the inputStyle into the textAreaStyle prop instead of the root style prop.

We would change it to something like this:

<EnhancedTextarea
  {...other}
  {...inputProps}
  // style={inputStyle} <- remove this line
  rows={rows}
  rowsMax={rowsMax}
  onHeightChange={this._handleTextAreaHeightChange}
  textareaStyle={inputStyle} // changed line
/>

This change means that we wouldn't make the root style for <EnhancedTextArea /> configurable. But I think most of the time the style the developer probably means to modify is really the text area, not the parent div. We may not be able to properly support modifying the root div of the <EnhancedTextArea /> until we support a more composable way to do this or (I really don't want to do this) add another prop to TextField for it.

@oliviertassinari
Copy link
Member

The style prop is usually used exclusively for the root element.

Good point. We should keep doing it 😁.

If not, I recommend we change TextField.jsx to not define any of the textarea styles and defined them all in EnhancedTextArea.

EnhancedTextarea is only used by the TextField. I think that it's a very good idea. styles.textarea is often defined and not used.

@oliviertassinari oliviertassinari 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 Feb 3, 2016
@newoga
Copy link
Contributor

newoga commented Mar 8, 2016

@third I'm going to close this for now. We really want to write tests and refactor TextField because we've been getting a lot of issues and PRs opened for it recently. I'll keep this on the radar and include when we're ready to make changes to the component.

Thanks much for this!

@newoga newoga closed this Mar 8, 2016
@newoga
Copy link
Contributor

newoga commented Mar 8, 2016

Actually @third, on reviewing an issue that was related to this, is there a reason why you couldn't use the textAreaStyles prop for carrying the color prop?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants