-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
feat: magic code input (passwordless login) #16076
feat: magic code input (passwordless login) #16076
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some minor comments, overall LGTM 🙌
@cristipaval @aimane-chnaif 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] |
Updated! |
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.
Minor comments.
Updated! |
reviewing today |
@aimane-chnaif flickering issue is created here #18218 |
I'm currently checking if the iOS 16.4 issue is related to the app code or is a RN/iOS bug. Will post updates when I find an answer |
It seems build is not generated again. We may have to test staging build on Testflight and also get feedback from QA team. |
@aimane-chnaif Screen.Recording.2023-05-02.at.12.14.05.movI've tried replicating in a new clean app with the latest RN version (0.71.7), I can't seem to replicate it. And using this latest version in the NewDot, the problem still keeps happening. The last thing I've tested was seeing if this was related to differences between old/new arch, but testing in RN-tester with the old arch, the problem was not replicated. I'm still not sure related to this last point, since I had some troubles with the setup though. Would have to spend more time on this. Just to clarify again, this problem only happens in iOS 16.4 (only tested in simulator). It seems like a RN issue however, even though I was not able to replicate it in rn-tester. |
@BeeMargarida thanks for the deep research. So you confirmed that this doesn't happen on 0.71.7. |
That's the weird part, when using 0.71.7 in the NewDot repo, the issue still remains. However, using the same version in a clean app, I can't replicate it. What makes me think this is a RN issue is that it happens even a simple |
🚀 Deployed to staging by https://github.com/cristipaval in version: 1.3.9-12 🚀
|
I'll be ooo today due to a national holiday, will continue migrating the component to function component tomorrow |
Alright, I also created a new issue for the migration to the function component. @BeeMargarida, please add a comment to that issue when you have time, so I can assign you to it. |
Thank you, done! |
🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.12-0 🚀
|
* | ||
* @param {Object} event | ||
*/ | ||
onKeyPress({nativeEvent: {key: keyValue}}) { |
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.
Methods should never be named for what they handle, they should only be named for what they do. This is part of the checklist:
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. toggleReport and not onIconClick)
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.
My bad, I must have skipped it when I read it. There's an issue regarding MagicCodeInput yet - #18325 - where this can be fixed, if I end up assigned, I can do it there.
* | ||
* @param {String} value | ||
*/ | ||
onChangeText(value) { |
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.
Same comment here about naming
#18736 was created because we weren't changing the border color to red, when there was an error. |
We added |
this.setState({ | ||
input: '', | ||
}); |
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.
Clearing input has caused this regression
This PR caused a regression here. Should have been caught while testing this offline, we should have considered the offline state before submitting post validation |
src/components/MagicCodeInput.js
Outdated
onFocus(event, index) { | ||
onFocus(event) { | ||
event.preventDefault(); | ||
this.setState({ | ||
input: '', | ||
}); | ||
} |
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.
Not setting the focused index onFocus caused a regression #23596
Details
Added a new component
MagicCodeInput
that is useful for the passwordless login. It contains 6 numeric inputs and supports copy paste.Fixed Issues
$ #15403
#15403 (comment)
Tests
Permissions.js
methodcanUsePasswordlessLogins
to returntrue
)Offline tests
Not applicable
QA Steps
Permissions.js
methodcanUsePasswordlessLogins
to returntrue
)PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)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
Screen.Recording.2023-03-31.at.18.11.19.mov
Mobile Web - Chrome
Screen.Recording.2023-03-31.at.18.22.06.mov
Mobile Web - Safari
Screen.Recording.2023-03-31.at.18.13.54.mov
Desktop
Screen.Recording.2023-03-31.at.18.12.52.mov
iOS
Screen.Recording.2023-03-31.at.18.10.44.mov
Android
Screen.Recording.2023-03-31.at.18.20.43.mov