-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Add a focused style to the composer box #520
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
Conversation
| } | ||
|
|
||
| componentDidUpdate(prevProps) { | ||
| this.focusInput(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this one because whenever the composer is rendered it focus the text input again. I don't think that is the desired behaviour. Specially with the purpose of this PR
| * | ||
| * @param {boolean} shouldHighlight | ||
| */ | ||
| focusTextInput(shouldHighlight) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest renaming this method to setIsFocused since this method doesn't actually focus the text input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| }, | ||
|
|
||
| chatItemComposeBoxFocusedColour: { | ||
| borderColor: colors.blue, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment makes it sound like we don't want blue, right @shawnborton?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I forgot to update the GH for the expensify one here
it was agreed later that blue was the better option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blue it is!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah missed that one, thanks!
| } | ||
|
|
||
| /** | ||
| * updates the Highlight state of the composer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * updates the Highlight state of the composer | |
| * Updates the Highlight state of the composer |
src/style/StyleSheet.js
Outdated
| display: 'flex', | ||
| }, | ||
|
|
||
| chatItemComposeBoxColour: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| chatItemComposeBoxColour: { | |
| chatItemComposeBoxColor: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/style/StyleSheet.js
Outdated
| borderColor: colors.border, | ||
| }, | ||
|
|
||
| chatItemComposeBoxFocusedColour: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| chatItemComposeBoxFocusedColour: { | |
| chatItemComposeBoxFocusedColor: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Stale review, changes were addressed and code looks good now!
fixes: https://github.com/Expensify/Expensify/issues/141643
Problem
the composer text input should have a focused style
Tests