[HOLD][$250] Dev - Getting console warning in android

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. build the app
2. immediately see the errors

Expected Result:

No console warnings displayed

Actual Result:

Console warnings about keyboardAvoidingViewBehavior are present

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Android

Version Number: 1.2.12-3
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Expensify/Expensify Issue URL:
Issue reported by: @aneequeahmad
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1665522948008369

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @MitchExpensify (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[aimane-chnaif]

Proposal

App/src/pages/home/ReportScreen.js

Line 210 in 299f07b

keyboardAvoidingViewBehavior={Platform.OS === 'android' ? '' : 'padding'}

The issue happens from above line added here

App/src/components/ScreenWrapper/propTypes.js

Line 24 in 299f07b

keyboardAvoidingViewBehavior: PropTypes.oneOf(['padding', 'height', 'position']),

keyboardAvoidingViewBehavior props value is expected one of these 3 but in android it's set to ''

Solution:

-               keyboardAvoidingViewBehavior={Platform.OS === 'android' ? '' : 'padding'}

remove this line
By doing this, android keyboardAvoidingViewBehavior set to height and iOS keyboardAvoidingViewBehavior keeps padding unchanged.

The default values are found here
ios:

App/src/components/ScreenWrapper/propTypes.js

Line 39 in 6bd5304

keyboardAvoidingViewBehavior: 'padding',

android:

App/src/components/ScreenWrapper/index.android.js

Line 14 in 6bd5304

defaultProps.keyboardAvoidingViewBehavior = 'height';

[hungvu193]

Actually, change the default proptypes can effect other screens.
For this case I think we should change:

App/src/pages/home/ReportScreen.js

Line 210 in 299f07b

keyboardAvoidingViewBehavior={Platform.OS === 'android' ? '' : 'padding'}

into:

-  keyboardAvoidingViewBehavior={Platform.OS === 'android' ? '' : 'padding'} 
+  keyboardAvoidingViewBehavior={Platform.OS === 'android' ? undefined : 'padding'} 

After that, the warning will disappear and also it won't effect other screens.

[thesahindia]

I vote we close this issue as it's not a big problem and will be fixed after #10648

[marcaaron]

Please wait until the issue has been triaged and Help Wanted label added.

[melvin-bot]

Current assignee @MitchExpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (External)

[melvin-bot]

Triggered auto assignment to @Luke9389 (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[marcaaron]

I vote we close this issue as it's not a big problem

I am not opposed to waiting if there's an open PR where it will be fixed, but let's leave this issue open until it is fixed. It's a problem, that's why I reported it, and we should fix it.

[parasharrajat]

Strangely, that component is using Platform API. We are discussing to revert it in #10648. I am not sure when that will be done.

@marcaaron Do you want me to look into proposals or hold this?

[marcaaron]

If we can fix this now it is fine to just do it.

[marcaaron]

We should generally not be merging PRs that introduce these kinds of warnings.

[parasharrajat]

I agree. Looks like there was discussion on the same https://github.com/Expensify/App/pull/10567/files#r955053952.

[parasharrajat]

@aimane-chnaif 's proposal looks good to me.

That line looks unnecessary to me as well.

cc: @Luke9389

[marcaaron]

cc @Luke9389 as I am not the assigned CME

[parasharrajat]

Oh Sorry, Didn't notice.

[Luke9389]

I think @aimane-chnaif's proposal to set the android value to height will cause this issue again (that's why we made this PR)

[parasharrajat]

So the android value is already set to height, it is just removing that warning.

[aimane-chnaif]

I don't find any issue on setting keyboardAvoidingViewBehavior=height in android. It doesn't cause any regression.
And it was me linking correct PR and comment exactly related to this GH.

keyboardAvoidingViewEnabled is a must-have props in ScreenWrapper in the future but still it requires platform specific code in this Report page which violates code philosophy.
keyboardAvoidingViewEnabled={Platform.OS === 'ios'}
or keyboardAvoidingViewEnabled={Platform.OS !== 'android'}

BTW, I am just notified this: #10273 (comment)
And this master PR removes that line as I proposed.
So I think we can wait for that PR to be merged and then go ahead the next step.

[Luke9389]

@aimane-chnaif, please be careful here. There is a known issue that we've linked to that is caused by using height. Are you really 100% positive that you checked that problem?

I do agree that we should wait on this PR to be merged. If the issue is still happening, I'd prefer to go with option 2 from @parasharrajat's comment here. @parasharrajat I don't like option 1 very much because it seems like an anti-pattern to have a component enable or disable itself (as opposed to just not using it in the parent).

For now I'd like to put this issue on HOLD to wait for that PR. Thoughts @parasharrajat?

[parasharrajat]

fine with me.

[Luke9389]

On HOLD.

[aneequeahmad]

I think i should also be added in reporter as I reported this bug on 1st September here and again same error on another component on 28th September here. Thanks

cc: @kavimuru, @MitchExpensify, @Luke9389

[MitchExpensify]

Looks fair to me @aneequeahmad! Added you as a reporter

[MitchExpensify]

I'm going to hold on the Upwork job while the issue is on hold

[Luke9389]

still on hold

[aimane-chnaif]

#11586 is merged already and the current codebase is what I proposed here.
Since this issue no longer happens, can be closed now.

[MitchExpensify]

@Luke9389 What do you think about closing this given the above comment?

[Luke9389]

Gotta read this one back over.

[Luke9389]

Yep we can close this now. Thanks @aimane-chnaif.

[aneequeahmad]

@MitchExpensify This one is fixed in production, am i eligible for reporting bonus ?

[MitchExpensify]

Yes @aneequeahmad, paying now

[MitchExpensify]

Mind applying here and I'll issue the reporting bonus? Thanks @aneequeahmad

[aneequeahmad]

@MitchExpensify applied thanks

[MitchExpensify]

Paid!