IOS/Android - Payments - Making payment method default will have keyboard overlapping password prompt #10162
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
There was a problem hiding this comment.
@dhairyasenjaliya keyboardspacer tests fine.
I'm leaving a general feedback since this is a draft PR.
You could choose to wait for this slack 🧵 to resolve before making any changes.
| } | ||
|
|
||
| componentDidMount() { | ||
| const updateListener = Platform.OS === 'android' ? 'keyboardDidShow' : 'keyboardWillShow'; |
There was a problem hiding this comment.
Please remove all platform specific code.
You could follow this code structure -
- BaseKeyboardSpacer
- index.ios.js
- index.android.js
A good example is https://github.com/Expensify/App/tree/main/src/components/Modal
There was a problem hiding this comment.
alright will follow this approach
src/components/PasswordPopover.js
Outdated
| @@ -92,6 +95,7 @@ class PasswordPopover extends Component { | |||
| onSubmitEditing={() => this.props.onSubmit(this.state.password)} | |||
| style={styles.mt3} | |||
| autoFocus | |||
| shouldDelayFocus={getPlatform() === CONST.PLATFORM.ANDROID} | |||
There was a problem hiding this comment.
please remove platform specific code
https://github.com/Expensify/App#platform-specific-file-extensions
There was a problem hiding this comment.
bit confused about this one I mean we should keep delay only for android so do I need to create separately files for all platform's
There was a problem hiding this comment.
@dhairyasenjaliya maybe give this a look, you'll notice how shouldDelayFocus is used for Android only with the help of index.android.js
Let me know if you have any questions :)
There was a problem hiding this comment.
I see, this is recent changes to allow shouldDelayFocus for only android will check and add according thank you :)
| this.input.focus(); | ||
| if (this.props.shouldDelayFocus) { | ||
| setTimeout(() => this.input.focus(), CONST.ANIMATED_TRANSITION); | ||
| } else { |
There was a problem hiding this comment.
we could return early here
There was a problem hiding this comment.
sure we can, I was just following the previous codebase how about we set 100ms ?
There was a problem hiding this comment.
We'll experiment it towards the end of the PR
| Keyboard, | ||
| LayoutAnimation, | ||
| View, | ||
| Dimensions, | ||
| ViewPropTypes, | ||
| Platform, | ||
| StyleSheet, |
There was a problem hiding this comment.
NAB, let's put these in alphabetical order.
…into Fix_issue_9928
|
@dhairyasenjaliya just a gentle reminder to mark this PR as ready for review after you're done with cleaning up the code |
Sure, this time I will require a bit of time due to new components changes so will wrap as soon as possible |
|
that's not a problem at all, thanks for letting us know!! |
@rushatgabhane do I need to commit package-lock.json since I removed KeyboardSpacer package ? |
|
@dhairyasenjaliya yepp |
|
but it creates different integrity is that fine for merging? |
i don't think so Make sure your npm version is 6.14.17 and then do the npm uninstall etc |
hm I have 6.14.9 do I need to downgrade ? |
melvin-bot
bot
requested review from
mountiny and
rushatgabhane
and removed request for
a team
|
alright we are good to go for review @rushatgabhane |
|
Regarding the freezeing issue you have raised here https://expensify.slack.com/archives/C01GTK53T8Q/p1659503910525829, does this PR also fix that? |
|
We could obviously increase the reward for this and also check if this issue has not been reported before. |
This PR doesn't solve that one since I believe that issue is related react native reanimation package has some issue with modal so will try to investigate more |
I'm pretty sure react-native-reanimated isn't being used in the app. It's just installed. Do you mean react-native's built-in Animated library? |
Yup I mean by react-native-reanimated sure we don't use it directly into app but this library is used by lot's of other packages e.g react navigation depends upon reanimated Im pretty sure that this could be the root cause for modal freez issue due to blocked touch point app stuck I'm checking what best we can put to solve will update soon |
|
@dhairyasenjaliya ohhhh TIL @dhairyasenjaliya if the fix is simple / related, let's do it in this PR. |
Sure I will check tomorrow morning first |
improve comments Co-authored-by: Carlos Martins <[email protected]>
…into Fix_issue_9928
There was a problem hiding this comment.
@dhairyasenjaliya Is it just me or the app freezes after the keyboard has been closed.
We also get this warning on closing the keyboard: Warning: Overriding previous layout animation with new one before the first began
Steps
- Payments page
- Click on any payment
- Make default
- Click somewhere outside to close the password modal
Screen.Recording.2022-08-21.at.2.34.30.PM.mov
There was a problem hiding this comment.
@dhairyasenjaliya please merge main into your branch as we've updated npm version
Yup that bug i have reported right before working on this PR Not on close keyboard but clicking outside the modal |
|
Oh yeah, totally forgot about that. Thanks for reminding me. @dhairyasenjaliya what do you think about the console warning? #10162 (review) |
Yeah that warning |
There was a problem hiding this comment.
Mentioning here because it's not in the diff
-
In
KeyboardSpacer/index.js
Please change this comment
On non-iOS platforms we do not need to implement a keyboard spacer, so we return a null component.
->
On non native platforms we do not need to implement a keyboard spacer, so we return a null component. -
Please delete
KeyboardSpacer/KeyboardSpacer.js
https://github.com/dhairyasenjaliya/Expensify/blob/Fix_issue_9928/src/components/KeyboardSpacer/KeyboardSpacer.js
There was a problem hiding this comment.
@dhairyasenjaliya In Tests/QA (Issue 9928)
Add a 5th step - "Verify that keyboard doesn't overlap the password text input"
|
@rushatgabhane added all requested changes |
There was a problem hiding this comment.
@dhairyasenjaliya this is a iOS - Safari only issue and works well on Android - Chrome.
Keyboard is overlapping the button - "Make default payment method".
We aren't modifying the web code, and it's also present on main.
Screen.Recording.2022-08-21.at.10.05.21.PM.mov
hm this is new let me check coz we have added this only for the native platform previously was working in non-native platform checking in new code |
|
@dhairyasenjaliya yeah, |
|
@rushatgabhane I did check the latest code actually it's not a bug let me explain So what I debug is when we are testing code in safari and by default, if we don't have a keyboard always open (command + k), and then we have a condition where there is autofocus the small amount of keyboard remains at the bottom and then you press (command + k) at that time this keyboard avoid is occurring
Result with keyboard always open (same as real device) actualResult.mp4Result without keyboard always open withoutAlwasyKeyboardOpen.mp4 |
There was a problem hiding this comment.
Excellent work and investigation @dhairyasenjaliya
@mountiny tests well on all platforms! 🎉
PR Reviewer Checklist
- I verified the correct issue is linked in the
### Fixed Issuessection above - I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the
Testssection - I verified the steps for Staging and/or Production testing are in the
QA stepssection - I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
- I verified the steps for local testing are in the
- I checked that screenshots or videos are included for tests on all platforms
- I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
- I verified there are no console errors (if there’s a console error not related to the PR, report it or open an issue for it to be fixed)
- I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e.
toggleReportand notonIconClick). - I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained “why” the code was doing something instead of only explaining “what” the code was doing.
- I verified any copy / text shown in the product was added in all
src/languages/*files - I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named “index.js”. All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in
STYLE.md) were followed
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e.
- If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
- I verified that this PR follows the guidelines as stated in the Review Guidelines
- I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
- I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
- If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a
/** comment above it */ - Any functional components have the
displayNameproperty - The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to
thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor) - Any internal methods bound to
thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick) - All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose and it is broken down into smaller components in order to separate concerns and functions
- If a new CSS style is added I verified that:
- A similar style doesn’t already exist
- The style can’t be created with an existing StyleUtils function (i.e.
StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
- If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like
Avataris modified, I verified thatAvataris working as expected in all cases) - If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
- I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.
There was a problem hiding this comment.
Great job @dhairyasenjaliya and thanks for such a thorough review @rushatgabhane!
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
Bug that seems related to this PR but is not (because reverting doesn't fix it) - https://expensify.slack.com/archives/C01GTK53T8Q/p1661203695542579 Please correct me if I'm wrong |
|
@rushatgabhane I checked that bug I dont think this is related here, as the composer never uses keyboardSpacer in the first place |
Details
IOS/Android - Payments - Making payment method default will have keyboard overlapping password prompt
<KeyboardSpacer />in order to avoid text input to hide behind the keyboard making component usable to the user<KeyboardSpacer />into our code base and removing the external library which was abandonedWhitespace on the screen where the keyboard used to be
<KeyboardSpacer />fromSidebarLinks.jsas this causes an issue for white space block UI at keyboard height which I explain here [HOLD for payment 2022-09-01] [$250] Whitespace on screen where keyboard used to be #10188 (comment)Fixed Issues
$ #9928
$ #10188
Tests (Issue 9928)
Tests (Issue 10188)
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly 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)PR Reviewer Checklist
The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */displayNamepropertythisproperly 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)QA Steps (Issue 9928)
QA Steps (Issue 10188)
Screenshots (Issue 9928)
Mobile Web
chrome_mobile.mov
safari_mobile.mp4
iOS
iphone_default_modal.mp4
Android
android_modal_fix.mp4
Desktop
Web
Screenshots (Issue 10188)
iOS
Fix_Issue.mp4
This issue only affects iOS so no other platform changes