-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Removed isVisible prop from ValidateCodeForm #21594
Removed isVisible prop from ValidateCodeForm #21594
Conversation
@rushatgabhane Please 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] |
@huzaifa-99 thank you for the quick PR 👍 I put a hold on the PR as we're on merge freeze right now until Wednesday. I'll review this PR soon though |
@hayata-suenaga Sure, Thank you for the heads up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM
Bumping for visibility @hayata-suenaga @rushatgabhane |
can you remove the hold please? |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-07-18.at.22.03.09.movMobile Web - ChromeWhatsApp.Video.2023-07-18.at.22.07.32.mp4Mobile Web - SafariScreen.Recording.2023-07-18.at.22.03.58.movDesktopScreen.Recording.2023-07-03.at.20.55.15.moviOSScreen.Recording.2023-07-03.at.21.12.26.movAndroidScreen.Recording.2023-07-18.at.20.13.42.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: the focused input in magic code stays at first position.
Merging with main
didn't fix it
Screen.Recording.2023-06-30.at.12.41.16.mov
@rushatgabhane fixed. Can you try again plz? Thank you! |
merge freeze is over. removing |
Bump @rushatgabhane |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@huzaifa-99 Bug android: keyboard doesn't stay open when navigating to code page.
Steps:
- On Android, Enter email
- Go next
Expected: keyboard stays open
Actual: keyboard is closed.
I verified that it stays open on main
Screen.Recording.2023-07-03.at.21.13.39.mov
@rushatgabhane Apologies for the late reply, I actually got caught up in testing and its painfully slow to run on M1 mac, don't know why. But I managed to test the main branch on android emulator, and the problem is present on Android Emulator M1Android.Emulator.M1.mp4I thought it might just be some cache issue so I cleared out every possible cache and retried 2-3 times, these were the caches I cleared
but still the same result. I had the intuition that it might be just an issue with Mac android emulator, to remove this doubt I tried on a windows machine, but that too had the same result Android Emulator WindowsAndroid.Emulator.Intel.mp4I cleared the same caches here as well and tried again but still no luck. And just to be sure, I tested the Android Physical DeviceAndriod.Physical.Device.mp4No matter which device I tried, the keyboard (if opened) got closed on android native after navigating to magic code form. With the testing above, I feel like the issue is also on |
@huzaifa-99 let me retest main. |
That would be great, thank you @rushatgabhane
maybe not sure, the code is pretty much the same i think |
@huzaifa-99 i retested on the latest Screen.Recording.2023-07-04.at.18.48.45.mov |
Thank you for testing @rushatgabhane, i will update here soon |
@huzaifa-99 thank you for the detailed explanation. Could you kindly tell me why the |
Thanks for having a look @hayata-suenaga. I don't know the exact reason why it happens but I am confident its some kind of race condition and falls in either or both of these categories
and this comment explains it a bit more. I will try to explain it a bit more from my understanding (apologies for this big paragraph below) A similar issue that got fixed 14 hours agoJust saw the latest main and Why this issue is with MagicCodeInput and not with our custom TextInputTo add, we are already using the In Both are text inputs but with different focus management techniques, the focus and keyboard works everywhere with one techniques but not with the other. Other remaining areas with the same issueWe still have 2 other pages that have the same issue
Proposed fixWe would have to add delayed focus to the first cell of Summary
If you guys agree then I can add the fix in this PR or maybe we can move those to a separate PR, whatever works best. |
@rushatgabhane can you check the message @huzaifa-99 sent above? 🙇 |
Bump @rushatgabhane |
1 similar comment
Bump @rushatgabhane |
friendly bump @rushatgabhane ^ |
@rushatgabhane bump on this msg |
@huzaifa-99 i agree! we can add something like Let's add a comment on why we're doing it also |
@huzaifa-99 but why is this issue not happening on |
@rushatgabhane I believe it's something related to the test device. The keyboard is not reliably opening up on Android, I got the issue on all devices I tried but it's not happening in your case, and I think some other people also reproduced the issue (like those who worked on #21415), maybe we can tag them to confirm if they face it in these screens?
In the meantime, I will add the |
Update: I merge the Screen.Recording.2023-07-13.at.3.52.07.PM.movQuestion: do we still add |
bump ^ |
Bump on this @rushatgabhane |
Bump @rushatgabhane |
if it's fixed by merging with main, they let's not add it. |
Sure, Thank you @rushatgabhane. I believe next step is to complete this? cc: @hayata-suenaga |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you @rushatgabhane |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/hayata-suenaga in version: 1.3.42-20 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.42-26 🚀
|
🚀 Deployed to staging by https://github.com/hayata-suenaga in version: 1.3.44-0 🚀
|
🚀 Deployed to production by https://github.com/marcaaron in version: 1.3.44-2 🚀
|
Details
Fixed Issues
$ #19787
PROPOSAL: #19787 (comment)
Tests
Offline tests
Same as 'Tests' section above
QA Steps
Same as 'Tests' section above
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Chrome
Web.Chrome.mp4
Safari
Web.Safari.mp4
Mobile Web - Chrome
mWeb.Chrome.mp4
Mobile Web - Safari
mWeb.Safari.mp4
Desktop
Desktop.Native.mp4
iOS
IOS.Native.mp4
Android
Android.Native.mp4