Skip to content
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

Adds background to label #5959

Merged
merged 7 commits into from
Oct 20, 2021

Conversation

AlfredoAlc
Copy link
Contributor

@AlfredoAlc AlfredoAlc commented Oct 20, 2021

@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

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

QA Steps

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

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

web.mov

Desktop

Desktop.mov

iOS

iOS.mp4

Android

Android.mov

@AlfredoAlc AlfredoAlc requested a review from a team as a code owner October 20, 2021 03:53
@MelvinBot MelvinBot requested review from timszot and removed request for a team October 20, 2021 03:53
@parasharrajat
Copy link
Member

parasharrajat commented Oct 20, 2021

I will review it shortly.

There are issues with this change.

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

@AlfredoAlc
Copy link
Contributor Author

AlfredoAlc commented Oct 20, 2021

@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
Copy link
Member

I will test it thoroughly and Let you know.

@parasharrajat
Copy link
Member

parasharrajat commented Oct 20, 2021

Here is the edge issue. Check the right corner.
Screenshot 2021-10-21 00:25:52

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.

Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

labelScale={this.state.labelScale}
/>
<>
{multiline ? (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also rather than ternary let's use this syntax:

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

position: 'absolute',
top: 0,
width: '100%',
height: 28,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please test it and it should not have any regressions.

@parasharrajat
Copy link
Member

parasharrajat commented Oct 20, 2021

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

@parasharrajat
Copy link
Member

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

@AlfredoAlc
Copy link
Contributor Author

@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
Copy link
Member

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

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

labelScale={this.state.labelScale}
/>
<>
{multiline ? (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also rather than ternary let's use this syntax:

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

@roryabraham
Copy link
Contributor

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
Copy link
Contributor

⚠️ ⚠️ 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
Copy link
Contributor Author

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.

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

image

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@AlfredoAlc AlfredoAlc Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

AlfredoAlc commented Oct 20, 2021

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

image

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)

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I think we are good here.

@roryabraham roryabraham merged commit 04e2d12 into Expensify:main Oct 20, 2021
@OSBotify
Copy link
Contributor

🚀 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
Copy link

@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
Copy link
Member

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

@isagoico
Copy link

isagoico commented Oct 21, 2021

@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
Copy link
Contributor

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

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

@OSBotify
Copy link
Contributor

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

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

@OSBotify
Copy link
Contributor

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants