Adds background to label #5959

AlfredoAlc · 2021-10-20T03:53:42Z

@parasharrajat

Details

Adds a background to the label, to avoid overlapping text when TextInput is multiline.

Added zIndex -1 to the TextInput, because is needed in iOS if this isn't set, the text still overlaps.

Fixed Issues

$ #4515
$ #5704

Tests

Add a large text to a multiline TextInput.
Select a value in TextInput with autofill enabled and multiline. (Label background should be white)
Select a value in `TextInput with autofill enabled and non-multiline. (Label background should be the same color as autofill background)

QA Steps

Add a large text to a multiline TextInput.
Select a value in TextInput with autofill enabled and multiline. (Label background should be white)
Select a value in `TextInput with autofill enabled and non-multiline. (Label background should be the same color as autofill background)

Tested On

Screenshots

Web

web.mov

Desktop

Desktop.mov

iOS

iOS.mp4

Android

Android.mov

parasharrajat · 2021-10-20T12:03:47Z

I will review it shortly.

There are issues with this change.

Text can be seen behind the label on the right end.
You have changed the gap between label and Value. (Major).

AlfredoAlc · 2021-10-20T18:16:09Z

@parasharrajat I change the gap between the label and value.. this is what you mean right?, let me know if this gap is correct so I can push this change.

Actual gap

Acutal.gap.mov

New gap

New.gap.mov

About the text seen on the right end, I can't reproduce this issue, could you tell me in which platform or in which input did you got this issue ??

parasharrajat · 2021-10-20T18:29:31Z

I will test it thoroughly and Let you know.

parasharrajat · 2021-10-20T18:57:57Z

Here is the edge issue. Check the right corner.

I change the gap between the label and value.. this is what you mean right?, let me know if this gap is correct so I can push this change.

Gap is fine.

parasharrajat

I don't completely agree with the approach here but as this is currently blocking the deployment so we can go for it.

parasharrajat · 2021-10-20T18:59:02Z

src/components/ExpensiTextInput/BaseExpensiTextInput.js

-                                    labelScale={this.state.labelScale}
-                                />
+                                <>
+                                    {multiline ? (


I don't feel good about this change but let's drop a comment here why we need this for multiline only and what it does.

Also rather than ternary let's use this syntax:

{multiline && <View style={styles.expensiTextInputLabelBackground} />}

parasharrajat · 2021-10-20T18:59:42Z

src/styles/styles.js

+        position: 'absolute',
+        top: 0,
+        width: '100%',
+        height: 28,


shouldn't this be the padding-top of the TextInput?

You're right, I will change it to the same value as paddingTop from the TextInput

parasharrajat · 2021-10-20T19:00:03Z

src/styles/styles.js

@@ -550,6 +559,7 @@ const styles = {
        paddingBottom: 8,
        paddingHorizontal: 11.5,
        borderRadius: variables.componentBorderRadiusNormal,
+        zIndex: -1,


Please test it and it should not have any regressions.

parasharrajat · 2021-10-20T19:02:11Z

Please upload screenshots for all platforms showcasing single line and multiline input, highlighted and non highlighted.

parasharrajat · 2021-10-20T19:03:39Z

cc: @roryabraham this is the solution we are tending towards #5924 .

AlfredoAlc · 2021-10-20T19:44:47Z

@parasharrajat I kept testing those edges on the right, I believe that are the scrolling bar style.. I managed to see it when changing the computer configuration, see attached video. Could that be it ??

Screen.Recording.2021-10-20.at.12.41.04.mov

parasharrajat · 2021-10-20T19:49:09Z

Yup. that one. Use overflow: hidden on the wrapper view and this will be gone for good.

roryabraham

Overall approach seems fine to me - still need to test before I'll approve.

roryabraham · 2021-10-20T20:04:01Z

src/components/ExpensiTextInput/BaseExpensiTextInput.js

-                                    labelScale={this.state.labelScale}
-                                />
+                                <>
+                                    {multiline ? (


Also rather than ternary let's use this syntax:

{multiline && <View style={styles.expensiTextInputLabelBackground} />}

roryabraham · 2021-10-20T20:07:50Z

Going to add the CP Staging label so we can CP the solution when it's ready and @AlfredoAlc has addressed the requests for changes.

github-actions · 2021-10-20T20:08:12Z

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

AlfredoAlc · 2021-10-20T20:30:21Z

I already made the required changes, also updated the videos with this changes for all platforms.

I couldn't find an autofill input with multiline.

roryabraham

This is subtle, but the label isn't vertically centered in the view:

roryabraham · 2021-10-20T20:36:30Z

src/components/ExpensiTextInput/BaseExpensiTextInput.js

+                                <>
+                                    {/* Adding this background to the label only for multiline text input,
+                                    to prevent text overlaping with label when scrolling */}
+                                    {multiline && <View style={styles.expensiTextInputLabelBackground} />}


Am I correct in thinking that the only reason we don't add this view for single-line inputs is because of the effect it causes when an input is auto-filled? Did you see or investigate this comment?

Yes, it is only set to multiline to avoid seeing the white background when it isn't multiline and it has autofill. And it look all the container blue(or whatever the color set depending on the browser).

I did investigate on it but there are some points to consider when trying to change the background set by the browser.

See #5704 (comment)

Okay, well I don't think we need to worry too much about the styles applied by the browser's auto-fill, so this is probably okay for now.

AlfredoAlc · 2021-10-20T20:43:00Z

This is subtle, but the label isn't vertically centered in the view:

Yes @roryabraham , initially I set the height of 28 so it looked centered, but @parasharrajat suggested using the same height of the paddingTop from the TextInput.
#5959 (comment)

roryabraham

Okay, I think we are good here.

(cherry picked from commit 04e2d12)

OSBotify · 2021-10-20T22:10:29Z

🚀 Cherry-picked to staging by @roryabraham in version: 1.1.8-6 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

isagoico · 2021-10-21T17:59:11Z

@roryabraham Was QAing this on Android but I don't have the same "Manage Members" as in the testing screenshots
Still I think this is a pass on Android on my side (the text fields expanded instead of having a scroller)

parasharrajat · 2021-10-21T18:13:10Z

@isagoico New UI will be pushed with #5726 PR.

isagoico · 2021-10-21T22:07:34Z

@roryabraham We see build 1.1.8-8 on iOS 🎉 🎉 We tested this and it was a pass too (same behaviour as in iOS)

OSBotify · 2021-10-23T00:40:56Z

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

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

OSBotify · 2021-10-23T01:32:06Z

🚀 Deployed to staging by @roryabraham in version: 1.1.8-10 🚀

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

OSBotify · 2021-10-28T20:56:58Z

🚀 Deployed to production by @roryabraham in version: 1.1.10-2 🚀

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

Adds background to label

562ad4e

AlfredoAlc requested a review from a team as a code owner October 20, 2021 03:53

MelvinBot requested review from timszot and removed request for a team October 20, 2021 03:53

Alingned closing bracket

cc08d6c

parasharrajat suggested changes Oct 20, 2021

View reviewed changes

Changes label background height to correct height

93a79ba

AlfredoAlc added 2 commits October 20, 2021 12:49

Adds comment explaining the use of label background

8635982

Adds overflow prop to textinput container

336c913

roryabraham reviewed Oct 20, 2021

View reviewed changes

roryabraham assigned AlfredoAlc Oct 20, 2021

roryabraham added the CP Staging label Oct 20, 2021

Modifies styling syntax

e6e6f89

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

d045e55

roryabraham requested changes Oct 20, 2021

View reviewed changes

roryabraham mentioned this pull request Oct 20, 2021

Invite member - Multiline text overlaps with label #5924

Closed

roryabraham approved these changes Oct 20, 2021

View reviewed changes

roryabraham merged commit 04e2d12 into Expensify:main Oct 20, 2021

github-actions bot pushed a commit that referenced this pull request Oct 20, 2021

Merge pull request #5959 from AlfredoAlc/alfredo-fix-textinput-area

8d01a65

(cherry picked from commit 04e2d12)

This was referenced Oct 20, 2021

🍒 Cherry pick PR #5959 to staging 🍒 #5968

Merged

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

Closed

botify mentioned this pull request Oct 23, 2021

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

Closed

OSBotify mentioned this pull request Oct 23, 2021

Deploy Checklist: New Expensify 2021-10-23 #6017

Closed

OSBotify mentioned this pull request Aug 16, 2022

Deploy Checklist: New Expensify 2022-08-04 #10253

Closed

rushatgabhane mentioned this pull request Mar 21, 2023

[HOLD for payment 2023-03-06] [$1000] Emoji tops are being cut off in the personal message field on invite members page #14674

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds background to label #5959

Adds background to label #5959

AlfredoAlc commented Oct 20, 2021 •

edited

Loading

parasharrajat commented Oct 20, 2021 •

edited

Loading

AlfredoAlc commented Oct 20, 2021 •

edited

Loading

parasharrajat commented Oct 20, 2021

parasharrajat commented Oct 20, 2021 •

edited

Loading

parasharrajat left a comment

parasharrajat Oct 20, 2021

roryabraham Oct 20, 2021

parasharrajat Oct 20, 2021

AlfredoAlc Oct 20, 2021

parasharrajat Oct 20, 2021

parasharrajat commented Oct 20, 2021 •

edited

Loading

parasharrajat commented Oct 20, 2021

AlfredoAlc commented Oct 20, 2021

parasharrajat commented Oct 20, 2021

roryabraham left a comment

roryabraham Oct 20, 2021

roryabraham commented Oct 20, 2021

github-actions bot commented Oct 20, 2021

AlfredoAlc commented Oct 20, 2021

roryabraham left a comment

roryabraham Oct 20, 2021

AlfredoAlc Oct 20, 2021 •

edited

Loading

roryabraham Oct 20, 2021

AlfredoAlc commented Oct 20, 2021 •

edited

Loading

roryabraham left a comment

OSBotify commented Oct 20, 2021

isagoico commented Oct 21, 2021

parasharrajat commented Oct 21, 2021

isagoico commented Oct 21, 2021 •

edited

Loading

OSBotify commented Oct 23, 2021

OSBotify commented Oct 23, 2021

OSBotify commented Oct 28, 2021

Adds background to label #5959

Adds background to label #5959

Conversation

AlfredoAlc commented Oct 20, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Desktop

iOS

Android

parasharrajat commented Oct 20, 2021 • edited Loading

AlfredoAlc commented Oct 20, 2021 • edited Loading

Actual gap

New gap

parasharrajat commented Oct 20, 2021

parasharrajat commented Oct 20, 2021 • edited Loading

parasharrajat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat commented Oct 20, 2021 • edited Loading

parasharrajat commented Oct 20, 2021

AlfredoAlc commented Oct 20, 2021

parasharrajat commented Oct 20, 2021

roryabraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roryabraham commented Oct 20, 2021

github-actions bot commented Oct 20, 2021

AlfredoAlc commented Oct 20, 2021

roryabraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlfredoAlc Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlfredoAlc commented Oct 20, 2021 • edited Loading

roryabraham left a comment

Choose a reason for hiding this comment

OSBotify commented Oct 20, 2021

isagoico commented Oct 21, 2021

parasharrajat commented Oct 21, 2021

isagoico commented Oct 21, 2021 • edited Loading

OSBotify commented Oct 23, 2021

OSBotify commented Oct 23, 2021

OSBotify commented Oct 28, 2021

AlfredoAlc commented Oct 20, 2021 •

edited

Loading

parasharrajat commented Oct 20, 2021 •

edited

Loading

AlfredoAlc commented Oct 20, 2021 •

edited

Loading

parasharrajat commented Oct 20, 2021 •

edited

Loading

parasharrajat commented Oct 20, 2021 •

edited

Loading

AlfredoAlc Oct 20, 2021 •

edited

Loading

AlfredoAlc commented Oct 20, 2021 •

edited

Loading

isagoico commented Oct 21, 2021 •

edited

Loading