fix: otp new styles and flow #1250
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
How to use the Graphite Merge QueueAdd the label graphite-merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
jcaleb4
requested review from
moldy530,
rthomare,
dancoombs,
mokok123,
dphilipson and
linnall
as code owners
| }, | ||
| onSuccess: () => { | ||
| if (isConnected) { | ||
| setAuthStep({ type: "complete" }); | ||
| setAuthStep({ ...authStep, status: AuthStepStatus.success }); | ||
| setTitleText(ls.loadingOtp.verified); |
There was a problem hiding this comment.
It looks to me like we might be able to derive the title text from the auth step, so we don't need to maintain state for it. Is that right?
There was a problem hiding this comment.
I think we need the state since the error text is not only updated by the auth step but also the input field which is a child component, the error can come from there too.
| setErrorText(errorText); | ||
|
|
||
| if (errorText) { | ||
| setTitleText(ls.loadingOtp.title); |
There was a problem hiding this comment.
Why does resetting the OTP only reset the title if there is error text?
There was a problem hiding this comment.
Just so we don't call that function in case there wasn't an error, in that case the text doesn't have to be updated, but since removing the conditional is not affecting the functionality I'll just do it.
Thanks
| className={`text-xs font-normal underline ${ | ||
| isOTPVerifying ? "text-fg-disabled" : "text-btn-primary" | ||
| }`} | ||
| style={isOTPVerifying ? { opacity: 1 } : {}} |
There was a problem hiding this comment.
For consistency, can we use the tailwind class opacity-100 instead of a style prop here?
| setTitleText(ls.loadingOtp.verified); | ||
| setTimeout(() => { | ||
| setAuthStep({ type: "complete" }); | ||
| }, 3000); |
There was a problem hiding this comment.
Let's extract this delay into a top-level constant in this file.
| type: "otp_verify"; | ||
| email: string; | ||
| error?: Error; | ||
| status?: AuthStepStatus | null; |
There was a problem hiding this comment.
The | null is a bit out of place compared to the rest of these types. Can we have it as just status?: AuthStepStatus?
There was a problem hiding this comment.
yeah I will replace the null with a base state, good feedback, thanks.
| focus:outline-none focus:border-active | ||
| ${!disabled && "bg-bg-surface-default text-fg-primary"} | ||
| ${!!errorText && "border-fg-critical"} | ||
| ${isVerified && "border-fg-success"} |
There was a problem hiding this comment.
This is maybe pretty nitpicky, but this can end up adding classes with names like "null", "undefined", or "false", which due to the global nature of CSS could maybe conceivably be classes defined in the surrounding app and therefore inadvertently add styling to this element. I know it's pretty unlikely someone would choose those classnames, but it would still be more correct to do these with ternaries, e.g.
${isVerified ? "border-fg-success" : ""}If writing that all the time is too burdensome, we can make ourselves a helper similar to clsx.
There was a problem hiding this comment.
No nitpicky at all, this is great feedback, thanks!
jcaleb4
requested review from
adamegyed,
howydev,
Zer0dot,
jaypaik and
Blu-J
as code owners
Pull Request Checklist
yarn test)sitefolder, and guidelines for updating/adding docs can be found in the contribution guide)feat!: breaking change)yarn lint:check) and fix any issues? (yarn lint:write)Here is a video sample of the changes.
NOTE: Styles and logic for the verified stage is complete in the code, however I am still working on how to show that view for a few seconds before redirecting to the authenticated home view.
This is a screenshot of the styles in the app:
Task: https://app.asana.com/0/1205598840815267/1209006976980114/f
Change: We were using the default outline style for focus elements, this is open to each browser or OS since all of them can display the focus differently; what we did here is disable the default focus style and set a custom style for when the input field is focused, this should display the same way in all browsers and OSs.
This also aligns better with the styles for the regular input field when focussed in other sections.
Chrome Before change:
iPhone Before change:
Chrome After change:
iPhone After change:
Task: https://app.asana.com/0/1205598840815267/1208982122598661/f
Change: Removing the "otp_completing" view step to keep the OTP input field in view while the validation happens.
On error the message will be shown bellow the input field.
On new focus after error, the error state will reset so the user can try again.
When the last value is set into the input, the input is disabled to prevent changes while validating.
ticket: https://app.asana.com/0/1205598840815267/1208982122598661/f
Figma: https://www.figma.com/design/bUj0hMD0o8c5UqYyz5kJ44/WS-Login-Modal?node-id=38-1928&p=f&m=dev
changes: Here I am adding an enum to be used instead of strings for the auth steps and flow, the reason is that this way we centralize the source of the options we have, we avoid typos and we also make sure to keep a better order while integrating new steps and flows.
We're also adding some important style changes to the OTP flow when we have a "verifying", "error" or initial stages, there is the implementation for the "verified" stage but I am still working on showing it since right now as soon as the code is validated the view changes to the home page I am still investigation on why is this the case or how to let us see the verified stage for at least 3 seconds before moving to the next screen but is taking more time than I was expecting.
PR-Codex overview
This PR focuses on enhancing the user authentication flow by introducing new status handling and UI updates for the OTP verification process, along with adding new color variants and improving the overall code structure.
Detailed summary
fg-successcolor variant inutils.test.tsandtheme.ts.AuthStepStatusenum incontext.tsfor better status management.LoadingOtpcomponent to handle new states and display messages.CompletingOtpcomponent.EmailNotReceivedDisclaimerto reflect OTP verification status.OTPInputprops and handling of verified state.