fix/14084 Bank account : The bottom section of Personal Information blinks when opening and closing DatePicker - #14270
Merged
Conversation
…links when opening and closing DatePicker
melvin-bot
Bot
requested review from
jasperhuangg and
mollfpr
and removed request for
a team
January 13, 2023 07:04
|
@jasperhuangg @mollfpr 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] |
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
Contributor
Author
|
Hi @mollfpr @jasperhuangg , the PR has been created and all checks have passed. Please help me review my PR. Thank you! |
mollfpr
requested changes
Jan 13, 2023
Contributor
Reviewer Checklist
Screenshots/VideosWeb14270.Web-Safari.mov14270.Web-Chrome.movMobile Web - Chrome14270.mWeb-Chrome.movMobile Web - Safari14270.mWeb-Safari.movDesktop14270.Desktop.moviOS14270.iOS.movAndroid14270.Android.mov |
mollfpr
approved these changes
Jan 13, 2023
| /** | ||
| * Ios will auto dismiss the keyboard when popover is opened and open again when it's closed | ||
| * We need the keyboardDidHide listener to make sure that the popover will only be opened after the keyboard is closed | ||
| */ |
Contributor
There was a problem hiding this comment.
We only use block comments for commenting function or add a description to propTypes and we can touch on the comment to more explicit about the issue is and how to resolve it.
We ensure open the popover is after the keyboard is hidden to avoid blinks at the bottom
caused by the keyboard opening and closing after dismissing the popover.
Issue: https://github.com/Expensify/App/issues/14084
mollfpr
requested changes
Jan 13, 2023
mollfpr
left a comment
mollfpr
left a comment
Contributor
There was a problem hiding this comment.
Whoops, approving by accident.
jasperhuangg
requested changes
Jan 13, 2023
jasperhuangg
left a comment
jasperhuangg
left a comment
Contributor
There was a problem hiding this comment.
few minor changes, thanks for getting this up!
Co-authored-by: Jasper Huang <jasperhuangg@gmail.com>
Co-authored-by: Jasper Huang <jasperhuangg@gmail.com>
Contributor
Author
|
Hi @mollfpr @jasperhuangg, I just fixed the comments. Please help to review it again. Thanks |
Contributor
|
Thanks for the update @tienifr! Just a minor change on the test description, let's update the case since the noticeable fix is only on iOS. |
Contributor
Author
|
@mollfpr Done. Thanks for pointing out that |
mollfpr
approved these changes
Jan 14, 2023
mollfpr
left a comment
mollfpr
left a comment
Contributor
There was a problem hiding this comment.
LGTM 👍
A note to improve, we should always attach all the platform recordings even though in this issue we all change a specific platform file but it is worth always all the platforms rather than having a regression.
Also, your branch name is too long fix/14084-bank-account-the-bottom-section-of-personal-information-blinks-when-opening-and-closing-datepicker 🤣
All yours @jasperhuangg
jasperhuangg
approved these changes
Jan 14, 2023
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Contributor
Performance Comparison Report 📊Significant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
|
22 tasks
Contributor
|
🚀 Deployed to staging by @jasperhuangg in version: 1.2.55-0 🚀
|
1 task
Contributor
|
🚀 Deployed to production by @AndrewGable in version: 1.2.55-0 🚀
|
Contributor
Regression !!!!I caught a regression from this PR: https://expensify.slack.com/archives/C049HHMV9SM/p1674144058692939 Solution diff --git a/src/components/DatePicker/index.ios.js b/src/components/DatePicker/index.ios.js
index 6f37202cf..960fa798c 100644
--- a/src/components/DatePicker/index.ios.js
+++ b/src/components/DatePicker/index.ios.js
@@ -41,6 +41,7 @@ class DatePicker extends React.Component {
showPicker(event) {
// Opens the popover only after the keyboard is hidden to avoid a "blinking" effect where the keyboard was on iOS
// See https://github.com/Expensify/App/issues/14084 for more context
+ this.initialValue = this.state.selectedDate;
if (!this.props.isKeyboardShown) {
this.setState({isPickerVisible: true});
return;
@@ -50,7 +51,6 @@ class DatePicker extends React.Component {
listener.remove();
});
Keyboard.dismiss();
- this.initialValue = this.state.selectedDate;
event.preventDefault();
}
May I be added as a contributor to this issue/PR? |
1 task
Contributor
Closed
2 tasks
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Details
The action
Keyboard.dismiss()andthis.setState({isPickerVisible: true})are executed asynchronous. Besides, Ios will dismiss the keyboard when popover is opened and open again when it's closed.This PR implement this logic: "If keyboard is opening => I'll dismiss keyboard and open popover in
keyboardDidHidelistener. Otherwise I just open popover".Fixed Issues
$ #14084
PROPOSAL: #14084 (comment)
Tests
1, Login with any account
2, Open Settings => Workspace => Connect Bank Account => Connect Manually => Enter valid information.
3, Tap on any TextInput then tap on DatePicker, tap outside to dismiss DatePicker.
4a. For iOS verify that the bottom section of the page isn't blinking after Date Picker closes.
4b. For other platforms make sure the Date Picker is working fine.
Offline tests
QA Steps
1, Login with any account
2, Open Settings => Workspace => Connect Bank Account => Connect Manually => Enter valid information.
3, Tap on any TextInput then tap on DatePicker, tap outside to dismiss DatePicker.
4a. For iOS verify that the bottom section of the page isn't blinking after Date Picker closes.
4b. For other platforms make sure the Date Picker is working fine.
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
Mobile Web - Chrome
mweb_chrome.mp4
Mobile Web - Safari
mweb_safari.mp4
Desktop
iOS
ios.1.mp4
Android
android.mp4