-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Change style for TextInput area #5704
Change style for TextInput area #5704
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
recheck |
@AlfredoAlc is it ready for review? |
@parasharrajat yes, it is ready to review |
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.
Why is package-lock.json
changed? It should have not been changed. please revert it.
And don't worry about the CLA checks. As soon as the assigned engineer will trigger the actions, they will be all ✔️ .
It was modified when ran I already revert the file. |
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.
LGTM. It will be merged after n6-hold is lifted.
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 can use the approach from the Move this condition from the
App/src/styles/utilities/spacing.js Lines 194 to 197 in bc3cfc2
|
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.
A minor lint issue otherwise looks good.
@@ -194,7 +193,7 @@ class BaseExpensiTextInput extends Component { | |||
placeholder={(this.state.isFocused || !label) ? placeholder : null} | |||
placeholderTextColor={themeColors.placeholderText} | |||
underlineColorAndroid="transparent" | |||
style={inputStyle} | |||
style={[inputStyle, !hasLabel && styles.pv0,]} |
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.
No Trailing comma here.
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.
Great, let me remove that comma.
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.
LGTM and tests well.
Ready for final review and merge when n6-hold is lifted.
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.
Code looks good to me, but you have conflicts now. Please resolve those and this should be ready for merge.
@AlfredoAlc, Great job getting your first Expensify/App pull request over the finish line! 🎉 I know there's a lot of information in our contributing guidelines, so here are some points to take note of 📝:
So it might take a while before you're paid for your work, but we typically post multiple new jobs every day, so there's plenty of opportunity. I hope you've had a positive experience contributing to this repo! 😊 |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
@AlfredoAlc Could you please create a new PR solving the regression? Yes, it seems |
@parasharrajat @marcaaron Yes, I will take a look to solve this regression |
Thank you. Please tag me so that I can test it. |
@parasharrajat I already created the new PR |
@AlfredoAlc Another regression I found is that label is messing up with the Multiline TextInput. Also, the right corner is overflowing the right edges. |
@parasharrajat I suggest adding a background to the label, since the label is an Screen.Recording.2021-10-15.at.14.40.05.movLet me know if this change is OK, so I can create a new PR with this solution. |
Sorry, I missed your comment, somehow. Initially, I was thinking the same but It does not look good. Do you have any other way of fixing it? We can rethink the original solution as well. Meanwhile, I will try to find something. |
@parasharrajat Sure, I will find another way to fix this.. I'll let you know here. |
@parasharrajat As much as I try to find a way to work it out, there are some points to consider, when Autofilling is used:
I will keep looking into a solution that works best, just keeping you updated. |
Thanks for the update. This is what I think is best:
How does that sound? |
@parasharrajat I will create a new PR with this approach. |
Thanks. You can comment on the main issue. |
🚀 Deployed to production by @roryabraham in version: 1.1.8-9 🚀
|
Details
Set
selectable
prop inExpensiTextInputLabel
tofalse
.Move
padding
props fromexpensiTextInputContainer
toexpensiTextInput
to prevent theTextInput
to switch between blur and focus when clicking in the text label.Fixed Issues
$ #4515
Tests
First Name
text input.text input container
, it shouldn't blur nor select the text input labelQA Steps
Tested On
Screenshots
Web
Desktop