Change style for TextInput area #5704

AlfredoAlc · 2021-10-06T21:35:44Z

Details

Set selectable prop in ExpensiTextInputLabel to false.
Move padding props from expensiTextInputContainer to expensiTextInput to prevent the TextInput to switch between blur and focus when clicking in the text label.

Fixed Issues

$ #4515

Tests

Open profile page.
Clear the First Name text input.
Focus the text input.
Press inside the text input container, it shouldn't blur nor select the text input label

QA Steps

Tested On

Screenshots

Web

Desktop

github-actions · 2021-10-06T21:36:03Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

AlfredoAlc · 2021-10-06T21:36:45Z

I have read the CLA Document and I hereby sign the CLA

parasharrajat · 2021-10-07T15:57:15Z

recheck

parasharrajat · 2021-10-07T17:07:21Z

@AlfredoAlc is it ready for review?

AlfredoAlc · 2021-10-07T17:48:05Z

@parasharrajat yes, it is ready to review

parasharrajat

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 ✔️ .

AlfredoAlc · 2021-10-07T18:41:57Z

It was modified when ran npm install, I forgot to revert that change.

I already revert the file.

parasharrajat

~~LGTM~~. It will be merged after n6-hold is lifted.

parasharrajat

Sorry, found a regression
Input on search page got affected by this and placeholder is not centered.

AlfredoAlc · 2021-10-07T21:06:49Z

I can use the approach from the View container, and use the condition to see if it doesn't has a label then remove the paddingTop and paddingBottom.

Move this condition from the View to the TextInput in the style prop.

App/src/components/ExpensiTextInput/BaseExpensiTextInput.js

Line 169 in bc3cfc2

!hasLabel && styles.pv0,

App/src/styles/utilities/spacing.js

Lines 194 to 197 in bc3cfc2

    
           pv0: { 
        
               paddingTop: 0, 
        
               paddingBottom: 0, 
        
           },

parasharrajat

A minor lint issue otherwise looks good.

parasharrajat · 2021-10-08T05:51:12Z

src/components/ExpensiTextInput/BaseExpensiTextInput.js

@@ -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,]}


No Trailing comma here.

Great, let me remove that comma.

parasharrajat

LGTM and tests well.

Ready for final review and merge when n6-hold is lifted.

timszot

Code looks good to me, but you have conflicts now. Please resolve those and this should be ready for merge.

OSBotify · 2021-10-12T23:06:00Z

@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 📝:

Now that your first PR has been merged, you can be hired for another issue. Once you've completed a few issues, you may be eligible to work on more than one job at a time.
Once your PR is deployed to our staging servers, it will undergo quality assurance (QA) testing. If we find that it doesn't work as expected or causes a regression, you'll be responsible for fixing it. Typically, we would revert this PR and give you another chance to create a similar PR without causing a regression.
Once your PR is deployed to production, we start a 7-day timer ⏰. After it has been on production for 7 days without causing any regressions, then we pay out the Upwork job. 💰

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! 😊

OSBotify · 2021-10-12T23:06:30Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

marcaaron · 2021-10-13T23:36:25Z

Pretty sure this caused a regression with labels on autofilled forms. Since they have a z-index of -1 they're now hidden

The autofill also expands beyond the edges but unsure if that was introduced in this PR.

marcaaron · 2021-10-13T23:37:16Z

Probably we want something more like this?

parasharrajat · 2021-10-14T09:09:50Z

@AlfredoAlc Could you please create a new PR solving the regression?

Yes, it seems z-index is causing the issue.

AlfredoAlc · 2021-10-14T17:06:41Z

@parasharrajat @marcaaron Yes, I will take a look to solve this regression

parasharrajat · 2021-10-14T17:41:18Z

Thank you. Please tag me so that I can test it.

AlfredoAlc · 2021-10-14T21:31:35Z

@parasharrajat I already created the new PR

OSBotify · 2021-10-15T06:40:28Z

🚀 Deployed to staging by @timszot in version: 1.1.7-25 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

parasharrajat · 2021-10-15T13:50:45Z

@AlfredoAlc Another regression I found is that label is messing up with the Multiline TextInput.

Also, the right corner is overflowing the right edges.

AlfredoAlc · 2021-10-15T22:44:27Z

@parasharrajat I suggest adding a background to the label, since the label is an Animated.Text if a background is set, it doesn't fill the whole space where it is, so I added a View with a white background. But, when it is autofilled the area of the label stays white. See attached video.

Screen.Recording.2021-10-15.at.14.40.05.mov

Let me know if this change is OK, so I can create a new PR with this solution.
Also, I couldn't reproduce the right corner overflowing.

parasharrajat · 2021-10-19T14:58:21Z

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.

AlfredoAlc · 2021-10-19T17:19:14Z

@parasharrajat Sure, I will find another way to fix this.. I'll let you know here.

AlfredoAlc · 2021-10-20T02:19:50Z

@parasharrajat As much as I try to find a way to work it out, there are some points to consider, when Autofilling is used:

The background color is only set to the TextInput. So the whole container would not get the background color unless the TextInput fills the container(The way it is right now).
Not all browsers and not all autofill types fire up an event( so it wouldn't be possible to know when autofill value is selected).
The label has an absolute position, due to its animation. So if a background is not set to the label, it will overlap with the TextInput value (The way it is right now).

I will keep looking into a solution that works best, just keeping you updated.

parasharrajat · 2021-10-20T02:41:23Z

Thanks for the update.
@AlfredoAlc I tried myself to find a solution. we won't be able to fill the complete background color for autofill.

This is what I think is best:

Only for multiline inputs, implement this Change style for TextInput area #5704 (comment).

How does that sound?
This regression is blocking deployment so we can move with the solution and I don't think other is possible. I checked other libs and none of them have an autofill background on the full container.

AlfredoAlc · 2021-10-20T02:53:00Z

@parasharrajat I will create a new PR with this approach.

parasharrajat · 2021-10-20T02:57:27Z

Thanks. You can comment on the main issue.

OSBotify · 2021-10-23T00:42:50Z

🚀 Deployed to production by @roryabraham in version: 1.1.8-9 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

Change style for TextInput area

b796b34

AlfredoAlc requested a review from a team as a code owner October 6, 2021 21:35

MelvinBot requested review from timszot and removed request for a team October 6, 2021 21:35

parasharrajat suggested changes Oct 7, 2021

View reviewed changes

AlfredoAlc added 2 commits October 7, 2021 11:26

Restore commit

abdea5b

Restore package-lock file

31bd3c0

parasharrajat approved these changes Oct 7, 2021

View reviewed changes

parasharrajat suggested changes Oct 7, 2021

View reviewed changes

Move hasLabel condition to text input style

714aedf

parasharrajat suggested changes Oct 8, 2021

View reviewed changes

Trailing comma delete

3232f95

parasharrajat previously approved these changes Oct 8, 2021

View reviewed changes

timszot suggested changes Oct 12, 2021

View reviewed changes

Merge branch 'main' into alfredo-fix-textinput-area

57dde06

AlfredoAlc dismissed parasharrajat’s stale review via 57dde06 October 12, 2021 21:34

parasharrajat approved these changes Oct 12, 2021

View reviewed changes

timszot approved these changes Oct 12, 2021

View reviewed changes

timszot merged commit 50beee9 into Expensify:main Oct 12, 2021

AlfredoAlc mentioned this pull request Oct 14, 2021

Remove pointer events from label, modify styling of textinput #5858

Merged

5 tasks

OSBotify mentioned this pull request Oct 15, 2021

Deploy Checklist: New Expensify 2021-10-15 #5879

Closed

parasharrajat mentioned this pull request Oct 15, 2021

Input switch between focus and blurr state on click & label is selected with respect to input text #4515

Closed

parasharrajat mentioned this pull request Oct 18, 2021

Invite member - Multiline text overlaps with label #5924

Closed

parasharrajat mentioned this pull request Oct 20, 2021

Fix multiline ExpensiTextInput label #5958

Closed

5 tasks

AlfredoAlc mentioned this pull request Oct 20, 2021

Adds background to label #5959

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change style for TextInput area #5704

Change style for TextInput area #5704

AlfredoAlc commented Oct 6, 2021

github-actions bot commented Oct 6, 2021 •

edited

Loading

AlfredoAlc commented Oct 6, 2021

parasharrajat commented Oct 7, 2021

parasharrajat commented Oct 7, 2021

AlfredoAlc commented Oct 7, 2021

parasharrajat left a comment •

edited

Loading

AlfredoAlc commented Oct 7, 2021

parasharrajat left a comment •

edited

Loading

parasharrajat left a comment •

edited

Loading

AlfredoAlc commented Oct 7, 2021 •

edited

Loading

parasharrajat left a comment

parasharrajat Oct 8, 2021

AlfredoAlc Oct 8, 2021

parasharrajat left a comment •

edited

Loading

timszot left a comment

OSBotify commented Oct 12, 2021

OSBotify commented Oct 12, 2021

marcaaron commented Oct 13, 2021

marcaaron commented Oct 13, 2021

parasharrajat commented Oct 14, 2021 •

edited

Loading

AlfredoAlc commented Oct 14, 2021

parasharrajat commented Oct 14, 2021

AlfredoAlc commented Oct 14, 2021

OSBotify commented Oct 15, 2021

parasharrajat commented Oct 15, 2021

AlfredoAlc commented Oct 15, 2021

parasharrajat commented Oct 19, 2021 •

edited

Loading

AlfredoAlc commented Oct 19, 2021

AlfredoAlc commented Oct 20, 2021

parasharrajat commented Oct 20, 2021

AlfredoAlc commented Oct 20, 2021

parasharrajat commented Oct 20, 2021

OSBotify commented Oct 23, 2021

Change style for TextInput area #5704

Change style for TextInput area #5704

Conversation

AlfredoAlc commented Oct 6, 2021

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Desktop

github-actions bot commented Oct 6, 2021 • edited Loading

AlfredoAlc commented Oct 6, 2021

parasharrajat commented Oct 7, 2021

parasharrajat commented Oct 7, 2021

AlfredoAlc commented Oct 7, 2021

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

AlfredoAlc commented Oct 7, 2021

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

AlfredoAlc commented Oct 7, 2021 • edited Loading

parasharrajat left a comment

Choose a reason for hiding this comment

parasharrajat Oct 8, 2021

Choose a reason for hiding this comment

AlfredoAlc Oct 8, 2021

Choose a reason for hiding this comment

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

timszot left a comment

Choose a reason for hiding this comment

OSBotify commented Oct 12, 2021

OSBotify commented Oct 12, 2021

marcaaron commented Oct 13, 2021

marcaaron commented Oct 13, 2021

parasharrajat commented Oct 14, 2021 • edited Loading

AlfredoAlc commented Oct 14, 2021

parasharrajat commented Oct 14, 2021

AlfredoAlc commented Oct 14, 2021

OSBotify commented Oct 15, 2021

parasharrajat commented Oct 15, 2021

AlfredoAlc commented Oct 15, 2021

parasharrajat commented Oct 19, 2021 • edited Loading

AlfredoAlc commented Oct 19, 2021

AlfredoAlc commented Oct 20, 2021

parasharrajat commented Oct 20, 2021

AlfredoAlc commented Oct 20, 2021

parasharrajat commented Oct 20, 2021

OSBotify commented Oct 23, 2021

github-actions bot commented Oct 6, 2021 •

edited

Loading

parasharrajat left a comment •

edited

Loading

parasharrajat left a comment •

edited

Loading

parasharrajat left a comment •

edited

Loading

AlfredoAlc commented Oct 7, 2021 •

edited

Loading

parasharrajat left a comment •

edited

Loading

parasharrajat commented Oct 14, 2021 •

edited

Loading

parasharrajat commented Oct 19, 2021 •

edited

Loading