feat: Add UI and controller for Opt in step #3390
Conversation
WalkthroughIn a spectacular display of digital prowess, this pull request introduces a comprehensive Spender Onboarding Opt-In Step component. The implementation revolutionizes the mobile number verification process, providing a robust, user-friendly interface for spenders to complete their onboarding journey. With state-driven rendering and precise form validation, the component transforms the mundane task of mobile verification into an elegant, interactive experience. Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…E-86cx2t82k-implementation-opt-in-1
…ylein/fyle-mobile-app into FYLE-86cx2t82k-implementation-opt-in-1
...nder-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.scss
Outdated
Show resolved
Hide resolved
...nder-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.scss
Outdated
Show resolved
Hide resolved
...pender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.ts
Show resolved
Hide resolved
...pender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.ts
Show resolved
Hide resolved
...pender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.ts
Show resolved
Hide resolved
...pender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.ts
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.ts (3)
129-144: 🧹 Nitpick (assertive)Smaller Method Splits, Thambi!
ThisgoBack()houses multiple transitions (edit number, success dismiss, skip flow). Splitting them into separate handle methods could enhance testability, as previously noted.
154-180: 🧹 Nitpick (assertive)Separate Success & Error Steps, Machi!
InsidesaveMobileNumber(), the success path is big, and the error path is big. Consider refactoring into smaller methods for clarity and easy testing.
248-269: 🧹 Nitpick (assertive)RazhaRozha OTP Verification, Anna!
Handling the OTP in a singleverifyOtp()method is good. Consider extracting success and error branches for unit test brilliance. Thalaiva style!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.html(1 hunks)src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.scss(1 hunks)src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.ts(1 hunks)src/app/fyle/spender-onboarding/spender-onboarding.module.ts(1 hunks)src/app/fyle/spender-onboarding/spender-onboarding.page.html(1 hunks)src/app/fyle/spender-onboarding/spender-onboarding.page.ts(2 hunks)
🔇 Additional comments (13)
src/app/fyle/spender-onboarding/spender-onboarding.module.ts (3)
10-11: Mind-Blowing Imports, Thalaiva!
These imports for SpenderOnboardingOptInStepComponent and NgOtpInputModule are the right moves to turbocharge the onboarding flow. Perfect placement, Magizhchi!
17-17: Systematic ordering of imports, Superstar!
Placing SharedModule here is neat, and including NgOtpInputModule ensures the OTP feature is firing on all cylinders. In the immortal words of Rajini: “Correct-ah, perfect-ah!”
Also applies to: 23-23
25-25: Declaration Dalapathi!
Adding SpenderOnboardingOptInStepComponent to declarations is the right call for modular usage. Rock on!
src/app/fyle/spender-onboarding/spender-onboarding.page.ts (2)
26-27: Thalaiva’s Seal of Approval on New Property
Declaring eou: ExtendedOrgUser; sets the stage for a flexible, future-proof structure, pa. Nicely done!
50-50: Confirm Data Flow, Boss!
Assigning this.eou = eou; and setting this.userFullName is splendid. Just ensure all references to this.eou are well-guarded against undefined surprises.
src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.ts (4)
39-40: Neat Usage of @ViewChild, Superstar!
Capturing mobileInputEl is a convenient reference for focusing or reading native input attributes. Perfect resourcefulness!
41-42: OTP Handling is Marana Mass!
@ViewChild(NgOtpInputComponent) ensures you can reset or automate OTP flows with ease. Keep the momentum, Thala!
146-152: Solid Validation, Boss!
Checking if the user typed the number and verifying against the +1 pattern is top-notch. This helps avoid nonsense inputs. Rock on!
287-297: Time-limited OTP Flow, Semma!
startTimer() approach is excellent. Displaying a countdown for the OTP is user-friendly and intuitive. Superstar status achieved!
src/app/fyle/spender-onboarding/spender-onboarding.page.html (2)
28-28: Fine Conditional For CONNECT_CARD, Machan!
The step-based approach helps maintain a crisp user journey. Each block is neatly isolated.
34-39: Opt-In Step is the Hero, I Say!
Conditionally rendering the app-spender-onboarding-opt-in-step is perfect. The (isStepCompleted) binding fosters a smooth exit. This is unstoppable, like Rajinikanth on screen!
src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.html (1)
79-89: Kabali da! This success message is perfect!
The success state implementation is as stylish as my walk! Clear messaging, proper hierarchy, and good use of icons.
src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.scss (1)
372-415: Thalaiva approves these success styles!
These styles are as perfect as my timing in action scenes! Good use of flexbox, proper spacing, and responsive design.
| </span> | ||
| </div> | ||
| <div class="opt-in-step__otp-container"> | ||
| <ng-otp-input (onInputChange)="onOtpChange($event)" [config]="otpConfig"></ng-otp-input> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Magizhchi! Let's enhance the OTP input accessibility!
The OTP input needs some power-packed accessibility features! Add aria attributes to help screen readers:
-<ng-otp-input (onInputChange)="onOtpChange($event)" [config]="otpConfig"></ng-otp-input>
+<ng-otp-input
+ (onInputChange)="onOtpChange($event)"
+ [config]="otpConfig"
+ aria-label="Enter verification code"
+ role="group"
+></ng-otp-input>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <ng-otp-input (onInputChange)="onOtpChange($event)" [config]="otpConfig"></ng-otp-input> | |
| <ng-otp-input | |
| (onInputChange)="onOtpChange($event)" | |
| [config]="otpConfig" | |
| aria-label="Enter verification code" | |
| role="group" | |
| ></ng-otp-input> |
| <div class="opt-in-step__primary-cta-container" *ngIf="optInFlowState === OptInFlowState.MOBILE_INPUT"> | ||
| <ion-button class="btn-primary opt-in-step__primary-cta" fill="clear" role="button" (click)="saveMobileNumber()"> | ||
| Continue | ||
| </ion-button> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mind it! The button needs loading state handling!
Just like how I handle my stunts with precision, we need to handle button states with care:
<div class="opt-in-step__primary-cta-container" *ngIf="optInFlowState === OptInFlowState.MOBILE_INPUT">
- <ion-button class="btn-primary opt-in-step__primary-cta" fill="clear" role="button" (click)="saveMobileNumber()">
+ <ion-button
+ class="btn-primary opt-in-step__primary-cta"
+ fill="clear"
+ role="button"
+ (click)="saveMobileNumber()"
+ [disabled]="isLoading"
+ [loading]="isLoading">
Continue
</ion-button>
</div>Committable suggestion skipped: line range outside the PR's diff.
| <input | ||
| #mobileInput | ||
| matInput | ||
| [(ngModel)]="mobileNumberInputValue" | ||
| class="opt-in-step__mobile-input-container__input" | ||
| [ngClass]="{ 'opt-in-step__mobile-input-container__input__error': mobileNumberError?.length }" | ||
| [placeholder]="'Enter mobile number with country code e.g +155512345..'" | ||
| type="tel" | ||
| [maxlength]="15" | ||
| [minlength]="7" | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mind it! Let's make this input field more accessible, partner!
The mobile input needs some style, just like my signature moves! Add these accessibility enhancements:
<input
#mobileInput
matInput
[(ngModel)]="mobileNumberInputValue"
class="opt-in-step__mobile-input-container__input"
[ngClass]="{ 'opt-in-step__mobile-input-container__input__error': mobileNumberError?.length }"
- [placeholder]="'Enter mobile number with country code e.g +155512345..'"
+ [placeholder]="'+1 (555) 123-4567'"
+ aria-label="Mobile number with country code"
+ [attr.aria-invalid]="mobileNumberError?.length > 0"
+ [attr.aria-describedby]="mobileNumberError?.length ? 'mobile-error' : null"
type="tel"
[maxlength]="15"
[minlength]="7"
/>Committable suggestion skipped: line range outside the PR's diff.
| padding: 6px 0; | ||
| border-bottom: 1px solid $grey-lighter; | ||
| font-size: 18px; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Style it like a boss! Let's make this input more finger-friendly!
For mobile inputs, we need more padding, just like how I need more space for my signature moves:
- padding: 6px 0;
+ padding: 12px 0;
border-bottom: 1px solid $grey-lighter;
font-size: 18px;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| padding: 6px 0; | |
| border-bottom: 1px solid $grey-lighter; | |
| font-size: 18px; | |
| padding: 12px 0; | |
| border-bottom: 1px solid $grey-lighter; | |
| font-size: 18px; |
| &__otp-container { | ||
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; | ||
| margin-bottom: 32px; | ||
|
|
||
| &__label { | ||
| margin: 0 8px 0 0; | ||
| color: $black-light; | ||
| line-height: 1.3; | ||
| font-weight: 400; | ||
| display: flex; | ||
| justify-content: space-between; | ||
| align-items: center; | ||
|
|
||
| &__attempts { | ||
| font-size: 12px; | ||
| margin-left: 8px; | ||
| } | ||
|
|
||
| &__resend { | ||
| color: $brand-primary; | ||
| font-size: 14px; | ||
| font-weight: 500; | ||
| background: none; | ||
|
|
||
| &__disabled { | ||
| @extend .opt-in-step__otp-container__label__resend; | ||
| opacity: 0.6; | ||
| } | ||
| } | ||
|
|
||
| &__otp-timer { | ||
| font-size: 14px; | ||
|
|
||
| &__timer { | ||
| color: $brand-primary; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| &__mandatory { | ||
| color: $red; | ||
| } | ||
|
|
||
| &__input { | ||
| border: 0; | ||
| border-radius: 0; | ||
| font-weight: 400; | ||
| line-height: 1.3; | ||
| color: $blue-black; | ||
| width: 100%; | ||
| padding: 8px 0; | ||
| border-bottom: 1px solid $grey-lighter; | ||
|
|
||
| &__error { | ||
| border-bottom: 1px solid $red; | ||
| } | ||
| } | ||
|
|
||
| &__input:focus { | ||
| border-bottom: 1px solid $blue-black; | ||
| } | ||
|
|
||
| &__error { | ||
| color: $red; | ||
| font-size: 12px; | ||
| } | ||
|
|
||
| &__info-box { | ||
| margin-bottom: 24px; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Time to punch up these styles with variables, partner!
Let's make these styles as organized as my movie scripts! Move repeated values to CSS variables:
+:root {
+ --otp-font-size-small: 12px;
+ --otp-font-size-medium: 14px;
+ --otp-spacing-base: 8px;
+ --otp-border-color: #{$grey-lighter};
+}
&__otp-container {
// ... existing styles ...
&__label {
- margin: 0 8px 0 0;
+ margin: 0 var(--otp-spacing-base) 0 0;
// ... rest of the styles ...
}Committable suggestion skipped: line range outside the PR's diff.
bistaastha
changed the base branch from
FYLE-86cx2t82k-implementation-connect-cards-2
to
FYLE-86cx2t82k-implementation-connect-cards-3
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.html (2)
Line range hint
67-91: Error messages need translation power, like my punch dialogues!Let's make these error messages more maintainable using Angular's translation service!
-<span *ngIf="fg.controls['card_number_' + card.id]?.errors.invalidCardNumber" - >Please enter a valid card number.</span> +<span *ngIf="fg.controls['card_number_' + card.id]?.errors.invalidCardNumber" + >{{ 'CARD.ERRORS.INVALID_NUMBER' | translate }}</span> -<span *ngIf="isVisaRTFEnabled && isMastercardRTFEnabled; else visaOnlyOrg" - >Enter a valid Visa or Mastercard number. If you have other cards, please contact your admin.</span> +<span *ngIf="isVisaRTFEnabled && isMastercardRTFEnabled; else visaOnlyOrg" + >{{ 'CARD.ERRORS.INVALID_NETWORK_BOTH' | translate }}</span>
Line range hint
179-185: Button needs loading style, like my signature move!The button state handling is good, but let's add a loading state during card enrollment!
<ion-button class="btn-primary connect-card__primary-cta" fill="clear" - [disabled]="fg.invalid" + [disabled]="fg.invalid || isEnrolling" role="button" (click)="enrollCards()" > - Continue + <ng-container *ngIf="!isEnrolling">Continue</ng-container> + <ion-spinner *ngIf="isEnrolling" name="dots"></ion-spinner> </ion-button>
🛑 Comments failed to post (6)
src/app/fyle/spender-onboarding/spender-onboarding.page.scss (1)
23-25: 🧹 Nitpick (assertive)
Kabali says this hide class is straight and simple!
The
&__step-hideclass withdisplay: noneis like my signature move - clean, effective, and gets the job done! But remember one thing, if you need smooth transitions later, you might want to consider usingvisibility: hiddenoropacity: 0instead.Here's a style that would allow for smooth transitions:
&__step-hide { - display: none; + visibility: hidden; + opacity: 0; + transition: visibility 0s, opacity 0.3s linear; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.&__step-hide { visibility: hidden; opacity: 0; transition: visibility 0s, opacity 0.3s linear; }src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.scss (1)
46-49: 🧹 Nitpick (assertive)
Style the masked numbers like my signature sunglasses!
The styling matches other inputs perfectly, but let's make it even better!
Consider adding styles for masked numbers:
&__card-last-four { font-size: 18px; font-weight: 500; + &--masked { + color: $dark-grey; + letter-spacing: 2px; + } }Committable suggestion skipped: line range outside the PR's diff.
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.html (3)
97-173: 🛠️ Refactor suggestion
Time to punch duplications, thalaiva style!
I see we're repeating the card input logic. Let's create a reusable component to handle both single and multiple card scenarios!
Create a new component
card-input.component.ts:@Component({ selector: 'app-card-input', template: ` <div class="connect-card__input-inner-container" [ngClass]="{'connect-card__input-inner-container--error': isInvalid}" > <input [formControlName]="controlName" [placeholder]="placeholder" (input)="onInput.emit($event)" ... /> <!-- Card type icons --> </div> <!-- Error messages --> ` }) export class CardInputComponent { @Input() controlName: string; @Input() placeholder: string; @Output() onInput = new EventEmitter<any>(); // ... other inputs and outputs }Then simplify both sections to use this component:
-<div class="connect-card__input-inner-container" [ngClass]="..."> - <!-- Duplicate card input logic --> -</div> +<app-card-input + [controlName]="'card_number_' + card.id" + [placeholder]="'xxxx xxxx xxxx'" + (onInput)="onCardNumberUpdate(card, $event)" +></app-card-input>
21-35: 🧹 Nitpick (assertive)
Kabali says: Make it accessible for everyone!
The card input looks stylish, but let's make it more accessible for all our users!
<input class="smartlook-show connect-card__card-number-input pl-0" inputmode="numeric" mask="0000 0000 0000 0000" placeholder="xxxx xxxx xxxx" data-testid="card-number-input" formControlName="card_number_{{ card.id }}" + [attr.aria-label]="'Enter card number for card ' + (i + 1)" + [attr.aria-invalid]="fg.controls['card_number_' + card.id].invalid && fg.controls['card_number_' + card.id].touched" required (input)="onCardNumberUpdate(card, 'card_number_' + card.id)" />Committable suggestion skipped: line range outside the PR's diff.
1-7: 🧹 Nitpick (assertive)
Mind it! Loading state needs more style, macha!
The loading state check is good, but let's add a loading spinner to give our users a better experience when
cardsLoadingis true!-<div class="connect-card__body" *ngIf="cardsLoading === false && fg"> +<div class="connect-card__body"> + <ion-spinner *ngIf="cardsLoading" name="crescent"></ion-spinner> + <div *ngIf="!cardsLoading && fg">Committable suggestion skipped: line range outside the PR's diff.
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts (1)
2-2: 🛠️ Refactor suggestion
Yo, “ChangeDetectorRef” is unused!
The linter complains. Remove it if you don’t plan to use it, thalaivaa.Apply this diff to keep the code clean:
-import { - ChangeDetectorRef, +import { Component, EventEmitter, ...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.import { Component, EventEmitter, ...🧰 Tools
🪛 eslint
[error] 2-2: 'ChangeDetectorRef' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: Run linters
[failure] 2-2:
'ChangeDetectorRef' is defined but never used
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.html(1 hunks)src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.scss(1 hunks)
🔇 Additional comments (6)
src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.html (3)
45-45: Mind it! The OTP input needs accessibility superpowers!The OTP input component needs proper ARIA attributes, just like how I need my sunglasses!
94-98: Style it like a boss! The button needs loading state!Just like how I handle my action sequences with style, we need to handle button states with finesse!
18-28: 🛠️ Refactor suggestionMagizhchi! The mobile input needs more power!
Just like how I handle my stunts with precision, we need to handle mobile input with proper accessibility and validation!
<input #mobileInput matInput [(ngModel)]="mobileNumberInputValue" class="opt-in-step__mobile-input-container__input" [ngClass]="{ 'opt-in-step__mobile-input-container__input__error': mobileNumberError?.length }" [placeholder]="'Enter mobile number with country code e.g +155512345..'" type="tel" [maxlength]="15" [minlength]="7" + pattern="^\+[1-9]\d{1,14}$" + required + aria-label="Mobile number with country code" + [attr.aria-invalid]="mobileNumberError?.length > 0" + [attr.aria-describedby]="mobileNumberError?.length ? 'mobile-error' : null" + autocomplete="tel" />Likely invalid or redundant comment.
src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.scss (3)
259-261: Style it like a boss! The input needs more breathing space!Just like how I need space for my signature moves, the input needs more padding!
289-361: Punch up these styles with variables, thalaiva!Let's organize these styles like my perfectly choreographed fight scenes!
76-76: 🧹 Nitpick (assertive)Mind it! Fix that double semicolon, partner!
Even small details matter in our style journey!
- padding: 16px 16px 8px;; + padding: 16px 16px 8px;Likely invalid or redundant comment.
| <div class="opt-in-step__heading">Opt in to send text receipts</div> | ||
| <div class="opt-in-step__sub-heading">This will help you send receipts via text message.</div> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mind it! Let's make these headings more accessible!
The headings need proper semantic structure and ARIA attributes, just like how I need my signature style!
-<div class="opt-in-step__heading">Opt in to send text receipts</div>
-<div class="opt-in-step__sub-heading">This will help you send receipts via text message.</div>
+<h1 class="opt-in-step__heading" role="heading" aria-level="1">Opt in to send text receipts</h1>
+<p class="opt-in-step__sub-heading" aria-label="Purpose">This will help you send receipts via text message.</p>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div class="opt-in-step__heading">Opt in to send text receipts</div> | |
| <div class="opt-in-step__sub-heading">This will help you send receipts via text message.</div> | |
| <h1 class="opt-in-step__heading" role="heading" aria-level="1">Opt in to send text receipts</h1> | |
| <p class="opt-in-step__sub-heading" aria-label="Purpose">This will help you send receipts via text message.</p> |
|
...pender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.ts
Show resolved
Hide resolved
bistaastha
merged commit 1e4cb72
into
FYLE-86cx2t82k-implementation-connect-cards-3
Note: Most of this PR is code directly taken from an existing component, with minor adjustments made to the content in the UI.
ClickUp: https://app.clickup.com/t/86cx2t7yx
Preview:
Summary by CodeRabbit
New Features
Improvements
Technical Updates