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

Remove unnecessary style causing bug in android #4807

Merged
merged 2 commits into from
Aug 25, 2021

Conversation

aldo-expensify
Copy link
Contributor

@aldo-expensify aldo-expensify commented Aug 24, 2021

cc: @pecanoro @marcaaron @jasperhuangg CC you guys because you have a good idea of what was going on.

Details

I labeled this CP Staging because it is fixing the following regression: #4730 (comment)

The style errorOutline was unnecessary and was causing an unwanted behaviour in Android.

Fixed Issues

$ #4730

Tests/QA

  1. Navigate to <baseURL>/bank-account/company.
  2. Input invalid data for the following fields and try to submit the form. Verify that red outlines AND growl messages occur.
    • Empty password
    • Invalid address street
    • Invalid address zip code
    • Invalid website URL
    • Invalid company tax ID
    • Invalid industry code
    • Invalid incorporation date

Make sure that in Android the value of the input with error doesn't disappear .

Expected to look like this:

Screen Shot 2021-08-24 at 1 31 52 PM

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

It was making the input value disappear when there was an error
@aldo-expensify aldo-expensify requested a review from a team August 24, 2021 20:30
@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.

@MelvinBot MelvinBot requested review from luacmartins and removed request for a team August 24, 2021 20:31
pecanoro
pecanoro previously approved these changes Aug 24, 2021
luacmartins
luacmartins previously approved these changes Aug 24, 2021
Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

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

Tests well and code LGTM!

@luacmartins
Copy link
Contributor

@aldo-expensify Feel free to self-merge once tests pass.

@marcaaron
Copy link
Contributor

The style errorOutline was unnecessary and was causing an unwanted behaviour in Android.

I get that it was unnecessary since it looks like we wrapped the TextInput in a View that already has an outline...

(hasError || errorText) && styles.borderColorDanger,

But is there some explanation for why this should make the element value completely disappear? 🤯

@aldo-expensify
Copy link
Contributor Author

@marcaaron I don't have good explanation for it, but what I found is:

  • The text content didn't really disappear, it was there but out of view. The input of is scrollable and if you scroll you eventually can see the text, but the space available for the text is very small:
Screen.Recording.2021-08-24.at.3.33.34.PM.mov
  • In Android we have to make the padding 0 (see styles.expensiTextInputAndroid). If use we a small padding value, i.e. 4:
    expensiTextInputAndroid: left => ({
        padding: 4,
        left,
    }),

The space for the input text becomes smaller and we get some scrolling:

Screen.Recording.2021-08-24.at.3.29.45.PM.mov
  • If we remove the padding: 0 completely from styles.expensiTextInputAndroid, the input value is not visible anymore (too much padding?).

I don't know why, but adding the style errorOutline to the TextInput seems to somehow mess up this in a similar way as adding padding messes it up. Maybe when we add the borderColor Android does something with the padding? 🤷 I just found that this fixed it by trial an error, removing and putting back things.

@aldo-expensify aldo-expensify dismissed stale reviews from luacmartins and pecanoro via 34f22b1 August 24, 2021 22:41
@aldo-expensify aldo-expensify requested a review from a team as a code owner August 24, 2021 22:41
@MelvinBot MelvinBot requested review from nickmurray47 and removed request for a team August 24, 2021 22:41
@aldo-expensify
Copy link
Contributor Author

aldo-expensify commented Aug 24, 2021

Update: had to resolve conflict and commit again. Tested again in web and Android and seems to work.

@aldo-expensify aldo-expensify merged commit 4112656 into main Aug 25, 2021
@aldo-expensify aldo-expensify deleted the aldo_fix-android-input-value-with-error branch August 25, 2021 01:42
github-actions bot pushed a commit that referenced this pull request Aug 25, 2021
…with-error

Remove unnecessary style causing bug in android

(cherry picked from commit 4112656)
@isagoico
Copy link

@aldo-expensify Can this be tested on your side? We're currently unable to navigate to /bank-account/company in Android or iOS bc we don't have access to the current URL in the app.

@aldo-expensify
Copy link
Contributor Author

aldo-expensify commented Aug 25, 2021

@aldo-expensify Can this be tested on your side? We're currently unable to navigate to /bank-account/company in Android or iOS bc we don't have access to the current URL in the app.

Sure, I'm waiting on the PR to be approved and merged.

roryabraham added a commit that referenced this pull request Aug 25, 2021
@roryabraham
Copy link
Contributor

After resolving conflicts during the CP, this PR is being deployed in 1.0.86-11 here

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.0.86-11 🚀

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

@aldo-expensify
Copy link
Contributor Author

Tested the stage branch locally with Android device and it is working

IOS failure?

@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging by @aldo-expensify in version: 1.0.86-11 🚀

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

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

@OSBotify
Copy link
Contributor

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

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 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.

7 participants