Fix TextField color by state logic #631
Conversation
src/components/inputs/TextField.js
Outdated
| @@ -330,7 +326,7 @@ export default class TextField extends BaseInput { | |||
| const {expandable, placeholder, placeholderTextColor, floatingPlaceholderColor, multiline} | |||
| = this.getThemeProps(); | |||
| const typography = this.getTypography(); | |||
| const placeholderColor = this.getStateColor(placeholderTextColor || DEFAULT_COLOR_BY_STATE.default); | |||
| const placeholderColor = this.getStateColor(placeholderTextColor || Colors.grey30); | |||
There was a problem hiding this comment.
Why to hard-code the color and not use the DEFAULT_COLOR_BY_STATE.default (that should be set to 'grey30' instead of 'grey10')?
There was a problem hiding this comment.
cause DEFAULT_COLOR_BY_STATE.default should be grey10 and not grey30
I'm assuming that DEFAULT_COLOR_BY_STATE is meant for the input text color
all the reset derive from it if needed or pass their own color if needed (like with the placeholder color)
There was a problem hiding this comment.
So why not hold a DEFAULT_PLACEHOLDER_COLOR_BY_STATE enum (like the DEFAULT_UNDERLINE_COLOR_BY_STATE) and get it from there instead of changing in 3 places?
There was a problem hiding this comment.
Cause technically the behavior is different.
in line 329 I have
const placeholderColor = this.getStateColor(placeholderTextColor || Colors.grey30);
in line 350 I have
outputRange: [placeholderColor, this.getStateColor(floatingPlaceholderColor || {focus: Colors.blue30, default: Colors.grey30})]
which is different
and in line 500 I have this
const placeholderColor = this.getStateColor(placeholderTextColor || Colors.grey30);
The 2nd case is different than the other cause we want the color transition when the state changes from focused to not-focused.
In the other cases (1+3), we don't want a different color for the different states. so I pass the Colors.grey30. If anything I can defined a single const for DEFAULT_PLACEHOLDER_COLOR
There was a problem hiding this comment.
I've defined DEFAULT_PLACEHOLDER_COLOR const
src/components/inputs/TextField.js
Outdated
| @@ -351,7 +347,7 @@ export default class TextField extends BaseInput { | |||
| }), | |||
| color: floatingPlaceholderState.interpolate({ | |||
| inputRange: [0, 1], | |||
| outputRange: [placeholderColor, this.getStateColor(floatingPlaceholderColor)] | |||
| outputRange: [placeholderColor, this.getStateColor(floatingPlaceholderColor || {focus: Colors.blue30, default: Colors.grey30})] | |||
There was a problem hiding this comment.
That was part of the bug with the colors.
The component assumed we have the same color states for placeholder and the input text color, but they're not the same according to the guidelines.
The color states for the input text is different than the color states of the placeholder
So basically, as I wrote in the previous comment DEFAULT_COLOR_BY_STATE represent the color states for the input text
and I pass a custom object for each part (placeholder, underline, etc..) according to our guidelines.
src/components/inputs/TextField.js
Outdated
| @@ -351,7 +347,7 @@ export default class TextField extends BaseInput { | |||
| }), | |||
| color: floatingPlaceholderState.interpolate({ | |||
| inputRange: [0, 1], | |||
| outputRange: [placeholderColor, this.getStateColor(floatingPlaceholderColor)] | |||
| outputRange: [placeholderColor, this.getStateColor(floatingPlaceholderColor || {focus: Colors.blue30, default: DEFAULT_PLACEHOLDER_COLOR})] | |||
There was a problem hiding this comment.
I don't understand, why DEFAULT_PLACEHOLDER_COLOR is not an enum with states if you have 'focus' and 'default' cases?
src/components/inputs/TextField.js
Outdated
| @@ -369,7 +365,7 @@ export default class TextField extends BaseInput { | |||
|
|
|||
| renderTitle() { | |||
| const {floatingPlaceholder, title, titleColor, titleStyle} = this.getThemeProps(); | |||
| const color = this.getStateColor(titleColor); | |||
| const color = this.getStateColor(titleColor || {focus: Colors.blue30, default: Colors.grey30}); | |||
There was a problem hiding this comment.
Why not use DEFAULT_PLACEHOLDER_COLOR here as well?
We had some issues (only in public) with the TextField colors
I went over it and decided to do a little refactor there (also refactor the tests)
In general, the logic should make more sense now and follows closely are UX guidelines