Fix: iOS button obstructed by keyboard #14392
Conversation
melvin-bot
bot
requested review from
sobitneupane and
tgolen
and removed request for
a team
|
@tgolen @sobitneupane One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Thanks for the update @rawalyogendra. I think I am going to ask you to still add videos for Web and Desktop platforms for sanity. |
|
So based on the videos attached I've got two questions:
|
@mananjadhav sure I will add the screen recordings of these platforms.
|
|
Cool. Thanks for clarifying. The changes are good and I've tested earlier when we discussed the issue. Let me test this again and complete the checklist |
|
@rawalyogendra I am testing this on my iPhone Simulator Safari but I can see the issue still exists. ios-failure-group-chat-keyboard.MP4 |
|
@mananjadhav I think we have not built and published the changes of our RNW fork to NPM, so we have to test it by pulling the latest changes from our RNW fork and build it locally. I have also added the missing screen recordings. |
Ah, is this something I should do? |
|
@flodnv yes, since we are using
|
|
I tried and failed 😅 Asked for help here: https://expensify.slack.com/archives/C01GTK53T8Q/p1674225100579739 |
|
I think I succeeded to publish version 0.18.10 https://www.npmjs.com/package/@expensify/react-native-web?activeTab=readme. It's my first time doing this so please lmk if something is wrong 🙇 |
|
@flodnv @mananjadhav I can confirm that the changes has been published to npm.
|
Screenshots/VideosWebweb-create-group-button.movMobile Web - Chromemweb-chrome-create-group-button.movMobile Web - Safarimweb-safari-create-group-button.movDesktopdesktop-create-group-button.moviOSios-create-group-button.movAndroidandroid-create-group-button.movThanks for the update @rawalyogendra. I was able to test and confirm it works, but I think we need to update the package.json with the correct version. So ideally we should have the Lines 43 to 45 in ae66a7f |
|
@rawalyogendra Waiting for your update on this one. Can you please update the version in package.json ? |
rawalyogendra
dismissed stale reviews from tgolen and mananjadhav
via
6a1ae16
|
@mananjadhav updated the RNW version. Sorry I misunderstood 😅 |
@flodnv I think we should follow up this as a separate issue. Also I just saw Thanks @sobitneupane for the help in linking the issues. |
|
@sobitneupane thank you for testing these out. @flodnv yeah, I think a separate issue makes sense at this point. |
|
I don't know why this Snyk check for new vulns is failing, it makes no sense. |
|
@sobitneupane please feel free to create a new bug report for what you found above, thanks! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
|
|
🚀 Deployed to staging by https://github.com/flodnv in version: 1.2.62-0 🚀
|
Reported it in slack |
|
🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.62-1 🚀
|
|
Note: There is a regression from this PR #14716 |
|
Thanks, I am not surprised 😞 |
|
I haven't dug into this PR very much, but is this depending on state to update the max height? If so we should consider using nativeProps or a ref so we can update it without depending on a re-render. |
| @@ -89,7 +89,7 @@ class ScreenWrapper extends React.Component { | |||
| paddingStyle, | |||
| ]} | |||
| > | |||
| <KeyboardAvoidingView style={[styles.w100, styles.h100]} behavior={this.props.keyboardAvoidingViewBehavior}> | |||
| <KeyboardAvoidingView style={[styles.w100, styles.h100, {maxHeight: this.props.windowHeight}]} behavior={this.props.keyboardAvoidingViewBehavior}> | |||
There was a problem hiding this comment.
@rawalyogendra I have a question about it. what is the reason maxHeight is set in KeyboardAvoidingView and not in the outermost View component? Do you think there will be a regression if we move the maxHeight to the outermost View component?
|
This PR introduced a regression in #17246, the pinch to zoom is not working after this PR is merged. |
Details
Fixed Issues
$ #11463
PROPOSAL: #11463 (comment)
$ #10670
PROPOSAL: #11463 (comment)
Tests
#11463
#10670
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.Screenshots/Videos
Web
Screen.Recording.2023-01-19.at.9.59.07.PM.mov
Mobile Web - Chrome
Record_2023-01-19-00-47-40.mp4
Mobile Web - Safari
Simulator.Screen.Recording.-.iPhone.13.-.2023-01-19.at.00.44.48.mp4
Desktop
Screen.Recording.2023-01-19.at.10.02.05.PM.mov
iOS
Simulator.Screen.Recording.-.iPhone.13.-.2023-01-19.at.00.10.59.mp4
Android
Record_2023-01-19-01-20-05.mp4