fix: Home page cleanup #3427
fix: Home page cleanup #3427
Conversation
sret-farhan2021
requested review from
Chethan-Fyle and
arjunaj5
as code owners
|
Warning Rate limit exceeded@sret-farhan2021 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughIn a whirlwind of code transformation, this pull request sweeps through multiple components, delivering stylistic and textual refinements across the Fyle dashboard. The changes primarily focus on enhancing user interface labels, improving component styling, and standardizing text capitalization. From card statistics to sidemenu items, the modifications bring a consistent and polished look to the application's user experience. Changes
Suggested labels
Suggested reviewers
Poem
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (2)
src/app/shared/components/card-number/card-number.component.html (1)
Line range hint
14-19: Add some punch dialogue to your unmasked digits section!Let's enhance the unmasked digits section with an ARIA label to make it more meaningful for screen readers.
<div *ngIf="cardNickname" class="card-number__unmasked-digits__with-nickname" - data-testid="unmasked-digits"> + data-testid="unmasked-digits" + [attr.aria-label]="'Last four digits of card number: ' + (cardNumber | slice : -4)"> {{ cardNumber | slice : -4 }} </div>src/app/shared/components/corporate-card/corporate-card.component.html (1)
Listen up, partner! Time to clean up this movie set like a boss!
That commented code block is like a deleted scene that's still hanging around in the final cut. The logos are being actively used in the add-corporate-card component, but with different logic. If you need AMEX support later, create a new task for it. For now, let's keep our code as stylish as my sunglasses - clean and clear!
🔗 Analysis chain
Line range hint
65-99: Mind it! Dead code is like a villain in my movie - it needs to go!If this code is no longer needed, remove it completely. If it's for future use, create a task to track it. Don't leave commented code hanging like a suspended fight scene!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if these logo assets are used elsewhere rg -l "amex-logo.png|visa-logo.png|mastercard-logo.png" --type html --type tsLength of output: 239
Script:
#!/bin/bash # Check the implementation in add-corporate-card component cat src/app/fyle/manage-corporate-cards/add-corporate-card/add-corporate-card.component.htmlLength of output: 7058
♻️ Duplicate comments (1)
src/app/shared/components/spent-cards/card-detail/card-detail.component.html (1)
78-78: 🛠️ Refactor suggestionSame story, different scene!
Hey, this is a duplicate of the issue above. When you have zero total expenses, it should still be "Expenses", not "Expense". Consistency is the super power of great UX!
Apply the same fix:
- Total {{ cardDetail.stats.totalTxnsCount <= 1 ? 'Expense' : 'Expenses' }} + Total {{ cardDetail.stats.totalTxnsCount === 1 ? 'Expense' : 'Expenses' }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
src/app/fyle/dashboard/card-stats/card-stats.component.html(2 hunks)src/app/fyle/dashboard/card-stats/card-stats.component.scss(2 hunks)src/app/fyle/dashboard/stat-badge/stat-badge.component.scss(1 hunks)src/app/fyle/dashboard/stats/stats.component.html(3 hunks)src/app/fyle/dashboard/stats/stats.component.scss(1 hunks)src/app/shared/components/card-number/card-number.component.html(1 hunks)src/app/shared/components/card-number/card-number.component.scss(1 hunks)src/app/shared/components/card-number/card-number.component.ts(1 hunks)src/app/shared/components/corporate-card/corporate-card.component.html(3 hunks)src/app/shared/components/corporate-card/corporate-card.component.scss(2 hunks)src/app/shared/components/sidemenu/sidemenu.component.ts(5 hunks)src/app/shared/components/spent-cards/card-detail/card-detail.component.html(2 hunks)src/theme/colors.scss(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/app/shared/components/card-number/card-number.component.html (1)
Learnt from: bistaastha
PR: fylein/fyle-mobile-app#3417
File: src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.html:25-25
Timestamp: 2025-01-15T07:10:18.310Z
Learning: In the card number input component for multiple cards, the mask can be set to "0000 0000 0000" (12 digits) when the last 4 digits are stored separately and combined during validation. This pattern enhances security by not requiring users to re-enter digits that are already known.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (12.x)
🔇 Additional comments (13)
src/app/shared/components/card-number/card-number.component.ts (1)
13-13: Mind it! The variable name change looks perfect, macha!The renaming from
maskedDotstomaskedAsterisksperfectly reflects the new visual representation. The array size of 12 is spot on for card number masking standards!src/app/shared/components/corporate-card/corporate-card.component.scss (1)
22-26: Mind it! This flex layout is perfectly balanced, like my stunts!The row class with flex properties creates a perfect harmony between space and alignment. When I say something works, it works!
src/app/shared/components/corporate-card/corporate-card.component.html (2)
4-9: Kabali da! This structure is as solid as my entry scene!The row organization with conditional bank name and badge is perfectly choreographed. This is how you write clean code!
59-61: 🧹 Nitpick (assertive)Time to simplify this like my famous one-liners!
The template expression can be made more powerful with a simple ternary operator.
<span class="corporate-card__feed-label">Synced using</span> - <span class="corporate-card__feed-label" data-testid="feed-info">{{ - card.data_feed_source === 'STATEMENT_UPLOAD' ? 'Statement upload' : 'Bank Feed' - }}</span> + <span class="corporate-card__feed-label" data-testid="feed-info"> + {{ card.data_feed_source === 'STATEMENT_UPLOAD' ? 'Statement upload' : 'Bank Feed' }} + </span>Likely invalid or redundant comment.
src/app/fyle/dashboard/card-stats/card-stats.component.scss (1)
21-22: Perfect spacing like a well-choreographed dance move!src/app/fyle/dashboard/stats/stats.component.scss (1)
25-34: Style consistency is like my signature move - it never fails!The header styling perfectly matches the card-stats component. That's how we maintain consistency, partner!
src/app/fyle/dashboard/card-stats/card-stats.component.html (1)
6-15: This header is like a perfectly timed entry scene - stylish and impactful!The icon addition and dynamic count make it more informative. The data-testid attribute shows you care about testing like a true professional!
src/theme/colors.scss (1)
132-132: Even a small color change can make a big impact, like my subtle expressions!The color update maintains the design system's harmony while improving the draft state's visibility.
src/app/fyle/dashboard/stats/stats.component.html (1)
23-23: Yo! Let's verify this enum change across the codebase!The change from 'DRAFT' to 'INCOMPLETE_EXPENSE' needs to be consistent everywhere.
src/app/shared/components/sidemenu/sidemenu.component.ts (4)
206-206: Superstar approves this style change!The title case update aligns with the new UI style guidelines.
226-226: Mind-blowing consistency across components, thalaiva!The "My expenses" title change perfectly matches with the stats component header, creating a unified experience.
240-247: Style makeover done with panache!The personalized titles "My reports" and "My advances" create a more engaging user experience.
Line range hint
322-336: Kabali style consistency maintained, boss!The title case updates for "Delegated accounts" and "Switch organization" complete the UI text refinement.
src/app/shared/components/spent-cards/card-detail/card-detail.component.html
Show resolved
Hide resolved
| <ng-container *ngFor="let item of maskedAsterisks; let index = index"> | ||
| <div | ||
| class="card-number__masked-digits__dot" | ||
| [ngClass]="{ 'card-number__masked-digits__fourth-dot': index % 4 === 3 }" | ||
| ></div> | ||
| class="card-number__masked-digits__asterisk" | ||
| [ngClass]="{ 'card-number__masked-digits__fourth-asterisk': index % 4 === 3 }" | ||
| > | ||
| * | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Kabali style template makeover! But let's add some ARIA magic!
The template transformation is stylish, but for better accessibility, let's add ARIA attributes to help screen readers understand this is a masked card number.
<ng-container *ngFor="let item of maskedAsterisks; let index = index">
<div
class="card-number__masked-digits__asterisk"
[ngClass]="{ 'card-number__masked-digits__fourth-asterisk': index % 4 === 3 }"
+ role="presentation"
+ aria-hidden="true"
>
*
</div>
</ng-container>📝 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-container *ngFor="let item of maskedAsterisks; let index = index"> | |
| <div | |
| class="card-number__masked-digits__dot" | |
| [ngClass]="{ 'card-number__masked-digits__fourth-dot': index % 4 === 3 }" | |
| ></div> | |
| class="card-number__masked-digits__asterisk" | |
| [ngClass]="{ 'card-number__masked-digits__fourth-asterisk': index % 4 === 3 }" | |
| > | |
| * | |
| </div> | |
| <ng-container *ngFor="let item of maskedAsterisks; let index = index"> | |
| <div | |
| class="card-number__masked-digits__asterisk" | |
| [ngClass]="{ 'card-number__masked-digits__fourth-asterisk': index % 4 === 3 }" | |
| role="presentation" | |
| aria-hidden="true" | |
| > | |
| * | |
| </div> |
| .corporate-card-badge-container { | ||
| display: flex; | ||
| align-items: center; | ||
| background-color: #ffffff; | ||
| border: 1px solid $grey-2; | ||
| border-radius: 5px; | ||
| max-height: 20px; | ||
| padding-left: 4px; | ||
| padding-right: 4px; | ||
| padding-top: 4px; | ||
| padding-bottom: 4px; | ||
| margin-left: auto; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Listen carefully! Let's make this code as smooth as my style!
The padding properties can be combined into one line, making it more powerful than my punch dialogues!
display: flex;
align-items: center;
background-color: #ffffff;
border: 1px solid $grey-2;
border-radius: 5px;
max-height: 20px;
- padding-left: 4px;
- padding-right: 4px;
- padding-top: 4px;
- padding-bottom: 4px;
+ padding: 4px;
margin-left: auto;📝 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.
| .corporate-card-badge-container { | |
| display: flex; | |
| align-items: center; | |
| background-color: #ffffff; | |
| border: 1px solid $grey-2; | |
| border-radius: 5px; | |
| max-height: 20px; | |
| padding-left: 4px; | |
| padding-right: 4px; | |
| padding-top: 4px; | |
| padding-bottom: 4px; | |
| margin-left: auto; | |
| } | |
| .corporate-card-badge-container { | |
| display: flex; | |
| align-items: center; | |
| background-color: #ffffff; | |
| border: 1px solid $grey-2; | |
| border-radius: 5px; | |
| max-height: 20px; | |
| padding: 4px; | |
| margin-left: auto; | |
| } |
There was a problem hiding this comment.
fix this @sret-farhan2021
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| <div class="card-stats__header"> | ||
| <div class="card-stats__header-content"> | ||
| <ion-icon | ||
| src="../../../../assets/svg/card.svg" | ||
| class="card-stats__logo--default__icon" | ||
| data-testid="default-icon" | ||
| ></ion-icon> | ||
| My cards | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Time to make this code as efficient as my punch dialogues!
Let's extract the repeated header into a reusable template to follow the DRY principle!
+<ng-template #cardHeader>
+ <div class="card-stats__header">
+ <div class="card-stats__header-content">
+ <ion-icon
+ src="../../../../assets/svg/card.svg"
+ class="card-stats__logo--default__icon"
+ data-testid="default-icon"
+ ></ion-icon>
+ My cards <ng-content></ng-content>
+ </div>
+ </div>
+</ng-template>
-<div class="card-stats__header">
- <div class="card-stats__header-content">
- <ion-icon
- src="../../../../assets/svg/card.svg"
- class="card-stats__logo--default__icon"
- data-testid="default-icon"
- ></ion-icon>
- My cards
- </div>
-</div>
+<ng-container *ngTemplateOutlet="cardHeader">
+ <span class="card-count" *ngIf="cardDetails?.length > 0"> ({{ cardDetails.length }})</span>
+</ng-container>Committable suggestion skipped: line range outside the PR's diff.
| <div class="stats--header"> | ||
| <div class="stats--header-content"> | ||
| <ion-icon | ||
| src="../../../../assets/svg/list.svg" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mind it! The relative path needs fixing, macha!
Using relative paths like "../../../../assets" can break when the component's location changes. Let's make it more robust!
- src="../../../../assets/svg/list.svg"
+ src="assets/svg/list.svg"📝 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.
| src="../../../../assets/svg/list.svg" | |
| src="assets/svg/list.svg" |
| <div class="stats--header"> | ||
| <div class="stats--header-content"> | ||
| <ion-icon | ||
| src="../../../../assets/svg/folder.svg" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Another relative path that needs our style, partner!
Same issue with relative path, let's fix this one too!
- src="../../../../assets/svg/folder.svg"
+ src="assets/svg/folder.svg"📝 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.
| src="../../../../assets/svg/folder.svg" | |
| src="assets/svg/folder.svg" |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/app/core/mock-data/sidemenu-item.data.ts(2 hunks)src/app/core/mock-data/sidemenu.data.ts(7 hunks)src/app/shared/components/corporate-card/corporate-card.component.spec.ts(2 hunks)src/app/shared/components/sidemenu/sidemenu.component.spec.ts(6 hunks)src/app/shared/components/sidemenu/sidemenu.component.ts(6 hunks)
🧰 Additional context used
📓 Learnings (1)
src/app/shared/components/corporate-card/corporate-card.component.spec.ts (1)
Learnt from: bistaastha
PR: fylein/fyle-mobile-app#3240
File: src/app/fyle/manage-corporate-cards/manage-corporate-cards.page.spec.ts:293-354
Timestamp: 2024-11-12T04:53:20.520Z
Learning: In 'src/app/fyle/manage-corporate-cards/manage-corporate-cards.page.spec.ts', the flags 'isVisaRTFEnabled$', 'isMastercardRTFEnabled$', and 'isYodleeEnabled$' do not have rapidly changing values, so tests for rapid changes in these flags are unnecessary.
🪛 Biome (1.9.4)
src/app/shared/components/sidemenu/sidemenu.component.ts
[error] 202-202: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/app/shared/components/sidemenu/sidemenu.component.spec.ts
[error] 226-226: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: build (12.x)
🔇 Additional comments (5)
src/app/shared/components/corporate-card/corporate-card.component.spec.ts (1)
275-275: Superstar approves this text change!Like a perfectly timed dialogue delivery, this test now matches the UI text perfectly! When style meets consistency, magic happens!
src/app/core/mock-data/sidemenu.data.ts (1)
13-13: Mind-blowing consistency in menu item naming, boss!The changes follow a superb pattern that enhances user experience:
- Personal items now start with "My" - making it crystal clear they belong to the user
- Compound words follow proper capitalization - maintaining professional style
Also applies to: 34-34, 47-47, 54-54, 79-79, 84-84, 100-100, 106-106, 113-113, 121-121, 131-131, 167-167, 173-173, 180-180, 187-187, 194-194
src/app/core/mock-data/sidemenu-item.data.ts (1)
24-24: Superstar level consistency maintained, thalaiva!The menu item titles are perfectly aligned with the changes in sidemenu.data.ts. This kind of consistency is what separates heroes from legends!
Also applies to: 29-29, 47-47
src/app/shared/components/sidemenu/sidemenu.component.ts (1)
201-201: Style makeover done with panache, boss!The menu titles have been updated consistently across all methods. Simply superb!
Also applies to: 206-206, 226-226, 240-240, 247-247, 305-305, 322-322, 336-336
src/app/shared/components/sidemenu/sidemenu.component.spec.ts (1)
224-224: Test cases updated like a boss!The test expectations have been perfectly synchronized with the menu title changes. When tests are this well-maintained, bugs don't dare to enter the code!
Also applies to: 230-230, 292-292, 321-321, 327-327, 334-334, 348-348, 353-353, 388-388, 405-405, 412-412
| // describe('card logo', () => { | ||
| // it('should show mastercard icon when the card is connected to mastercard RTF', () => { | ||
| // component.ngOnInit(); | ||
| // fixture.detectChanges(); | ||
|
|
||
| const icon = getElementBySelector(fixture, '[data-testid="mastercard-icon"]'); | ||
| expect(icon).toBeTruthy(); | ||
| }); | ||
| // const icon = getElementBySelector(fixture, '[data-testid="mastercard-icon"]'); | ||
| // expect(icon).toBeTruthy(); | ||
| // }); | ||
|
|
||
| it('should show visa icon when the card is connected to visa RTF', () => { | ||
| component.card = visaRTFCard; | ||
| component.isVisaRTFEnabled = true; | ||
| // it('should show visa icon when the card is connected to visa RTF', () => { | ||
| // component.card = visaRTFCard; | ||
| // component.isVisaRTFEnabled = true; | ||
|
|
||
| component.ngOnInit(); | ||
| fixture.detectChanges(); | ||
| // component.ngOnInit(); | ||
| // fixture.detectChanges(); | ||
|
|
||
| const icon = getElementBySelector(fixture, '[data-testid="visa-icon"]'); | ||
| expect(icon).toBeTruthy(); | ||
| }); | ||
| // const icon = getElementBySelector(fixture, '[data-testid="visa-icon"]'); | ||
| // expect(icon).toBeTruthy(); | ||
| // }); | ||
|
|
||
| it('should show amex icon when the card is connected to AMEX_FEED', () => { | ||
| component.card = amexFeedCard; | ||
| // it('should show amex icon when the card is connected to AMEX_FEED', () => { | ||
| // component.card = amexFeedCard; | ||
|
|
||
| component.ngOnInit(); | ||
| fixture.detectChanges(); | ||
| // component.ngOnInit(); | ||
| // fixture.detectChanges(); | ||
|
|
||
| const icon = getElementBySelector(fixture, '[data-testid="amex-icon"]'); | ||
| expect(icon).toBeTruthy(); | ||
| }); | ||
| // const icon = getElementBySelector(fixture, '[data-testid="amex-icon"]'); | ||
| // expect(icon).toBeTruthy(); | ||
| // }); | ||
|
|
||
| it('should show the default card icon when the card is not connected to visa, mastercard RTF and AMEX_FEED', () => { | ||
| component.card = statementUploadedCard; | ||
| // it('should show the default card icon when the card is not connected to visa, mastercard RTF and AMEX_FEED', () => { | ||
| // component.card = statementUploadedCard; | ||
|
|
||
| component.ngOnInit(); | ||
| fixture.detectChanges(); | ||
| // component.ngOnInit(); | ||
| // fixture.detectChanges(); | ||
|
|
||
| const icon = getElementBySelector(fixture, '[data-testid="default-icon"]'); | ||
| expect(icon).toBeTruthy(); | ||
| }); | ||
| }); | ||
| // const icon = getElementBySelector(fixture, '[data-testid="default-icon"]'); | ||
| // expect(icon).toBeTruthy(); | ||
| // }); | ||
| // }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mind it! Remove these commented tests instead of leaving them hanging!
Hey boss, commenting out tests is like leaving a movie halfway - it's not the Rajinikanth way! If these card logo tests are no longer needed because of UI changes, delete them completely. Clean code is like a well-choreographed fight scene - no unnecessary moves!
- // describe('card logo', () => {
- // it('should show mastercard icon when the card is connected to mastercard RTF', () => {
- ...
- // });
- // });| title: 'Team reports', | ||
| isVisible: allowedReportsActions && allowedReportsActions.approve && showTeamReportsPage, | ||
| route: ['/', 'enterprise', 'team_reports'], | ||
| }, | ||
| { | ||
| title: 'Team Advances', | ||
| title: 'Team advances', | ||
| isVisible: allowedAdvancesActions && allowedAdvancesActions.approve, | ||
| route: ['/', 'enterprise', 'team_advance'], | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Let's make the code bulletproof with optional chaining, thalaiva!
In the getTeamOptions method, we can enhance code safety using optional chaining.
- isVisible: allowedReportsActions && allowedReportsActions.approve && showTeamReportsPage,
+ isVisible: allowedReportsActions?.approve && showTeamReportsPage,📝 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.
| title: 'Team reports', | |
| isVisible: allowedReportsActions && allowedReportsActions.approve && showTeamReportsPage, | |
| route: ['/', 'enterprise', 'team_reports'], | |
| }, | |
| { | |
| title: 'Team Advances', | |
| title: 'Team advances', | |
| isVisible: allowedAdvancesActions && allowedAdvancesActions.approve, | |
| route: ['/', 'enterprise', 'team_advance'], | |
| }, | |
| title: 'Team reports', | |
| isVisible: allowedReportsActions?.approve && showTeamReportsPage, | |
| route: ['/', 'enterprise', 'team_reports'], | |
| }, | |
| { | |
| title: 'Team advances', | |
| isVisible: allowedAdvancesActions && allowedAdvancesActions.approve, | |
| route: ['/', 'enterprise', 'team_advance'], | |
| }, |
🧰 Tools
🪛 Biome (1.9.4)
[error] 202-202: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 207-207: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| @@ -11,7 +20,7 @@ | |||
| [currency]="homeCurrency$ | async" | |||
| [currencySymbol]="currencySymbol$ | async" | |||
| [name]="'Incomplete'" | |||
| [expenseState]="'DRAFT'" | |||
| [expenseState]="'INCOMPLETE_EXPENSE'" | |||
There was a problem hiding this comment.
what does this change do?
There was a problem hiding this comment.
different color state or else it overlaps color with DRAFT circle
| // const icon = getElementBySelector(fixture, '[data-testid="default-icon"]'); | ||
| // expect(icon).toBeTruthy(); | ||
| // }); | ||
| // }); |
There was a problem hiding this comment.
why are these commented out?
There was a problem hiding this comment.
have asked in thread wether should this be kept or not waiting for reply once recieved will delete it or just change and keep it
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/app/shared/components/corporate-card/corporate-card.component.scss (1)
131-136:⚠️ Potential issueHey partner! That line height is still shorter than my villain's screen time!
The line height needs to be fixed for better readability than my famous punch dialogues!
Apply this style fix, just like in the previous review:
color: $dark-grey; text-align: center; font-size: 11px; - line-height: 8px; + line-height: 13px;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/app/fyle/dashboard/card-stats/card-stats.component.html(2 hunks)src/app/fyle/dashboard/card-stats/card-stats.component.scss(2 hunks)src/app/fyle/dashboard/stats/stats.component.html(3 hunks)src/app/shared/components/corporate-card/corporate-card.component.html(2 hunks)src/app/shared/components/corporate-card/corporate-card.component.scss(2 hunks)src/app/shared/components/corporate-card/corporate-card.component.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (12)
src/app/shared/components/corporate-card/corporate-card.component.scss (2)
22-26: Mind it! This flex layout is as smooth as my style!The row class implementation is perfect, like a well-choreographed action sequence! Keep rocking with that flex power, partner!
120-129: Superstar move! You fixed the padding like a boss!The badge container styling is now perfect with that combined padding property. That's what I call a blockbuster implementation!
src/app/fyle/dashboard/card-stats/card-stats.component.scss (3)
7-7: Mind it! The color change is a style superstar!The switch to
$black-lightbrings better contrast and readability. That's how we roll in the world of style, macha!
12-14: When I do margin, even pixels salute! Kabali Da!That 2px margin-left is perfectly balanced, like all things should be!
44-66: Style panrom, mass panrom! (We do style, we do mass!)The logo styling needs the same power-packed improvements as mentioned before, for handling different screen sizes like a boss!
src/app/fyle/dashboard/card-stats/card-stats.component.html (2)
6-8: This header is like my punch dialogue - simple yet powerful!The dynamic card count shows up with style when needed. That's what I call perfect timing!
39-39: When you maintain consistency, success is not a question - it's an answer!The zero state header keeps the same style as its siblings. That's how you maintain order in the UI universe!
src/app/shared/components/corporate-card/corporate-card.component.spec.ts (2)
265-265: Looking good, partner! The text case change matches our style!The change from 'Statement Upload' to 'Statement upload' aligns perfectly with the component template.
Line range hint
1-265: Mind it! Those commented tests are still hanging around!Hey boss, commenting out tests is like leaving a movie halfway - it's not the Rajinikanth way! If these card logo tests are no longer needed because of UI changes, delete them completely. Clean code is like a well-choreographed fight scene - no unnecessary moves!
src/app/shared/components/corporate-card/corporate-card.component.html (1)
4-9: Stylish structure upgrade, macha!The new ion-row with bank name and corporate card badge brings a clean, organized look to our UI. That's the style!
src/app/fyle/dashboard/stats/stats.component.html (2)
5-7: Super style update, boss! Making it personal!Changing from "ALL EXPENSES" to "My expenses" and "EXPENSE REPORTS" to "My expense reports" makes the UI more friendly and personal. That's how we connect with our users!
Also applies to: 38-40
16-16: Smart move with that state change!Changing from 'DRAFT' to 'INCOMPLETE_EXPENSE' fixes the color overlap issue and better represents the expense state. Style ah style!
| <ng-container *ngIf="!isCardConnectedToRTF" class="corporate-card__logo--default"> | ||
| <ng-container *ngIf="card.data_feed_source === dataFeedSourceTypes.AMEX_FEED"> | ||
| <img | ||
| src="../../../../assets/images/amex-logo.png" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mind it! These relative paths need fixing, macha!
Using relative paths like "../../../../assets" can break when the component's location changes. Let's make it more robust!
- src="../../../../assets/images/amex-logo.png"
+ src="assets/images/amex-logo.png"Also applies to: 70-70
sret-farhan2021
force-pushed
the
FYLE-86cxq9pc7
branch
from
1ec5efd to
b00761b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/app/core/services/filters-helper.service.ts (1)
Line range hint
53-53: Watch out! TypeScript is showing its power!There's an unsafe assignment of 'any' value that needs to be fixed. Let's handle it with style!
- const { data } = (await filterPopover.onWillDismiss()) as { data: SelectedFilters<string | AdvancesStates[]>[] }; + const { data } = (await filterPopover.onWillDismiss<{ data: SelectedFilters<string | AdvancesStates[]>[] }>());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/app/core/mock-data/filter-options.data.ts(1 hunks)src/app/core/services/filters-helper.service.spec.ts(3 hunks)src/app/core/services/filters-helper.service.ts(1 hunks)src/app/fyle/my-advances/my-advances.page.ts(1 hunks)src/app/fyle/team-advance/team-advance.page.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Lint
src/app/core/services/filters-helper.service.ts
[error] 53-53: Unsafe assignment of an any value (@typescript-eslint/no-unsafe-assignment)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (5)
src/app/core/mock-data/filter-options.data.ts (1)
31-43: Mind it! These label changes are looking sharp!The transformation from "At" to "date" in sorting labels brings more clarity to the user interface. Keep rocking with these consistent changes!
src/app/core/services/filters-helper.service.spec.ts (1)
425-437: These test updates are perfectly synchronized!Like a well-choreographed fight scene, all the test cases have been updated to match the new label format. Superb coordination!
src/app/core/services/filters-helper.service.ts (1)
24-27: Style ah? Perfect ah? The labels are now crystal clear!The updated filter pill labels maintain consistency across the application. That's how we do it!
src/app/fyle/team-advance/team-advance.page.ts (1)
190-194: Mind-blowing transformation of these labels!Like a perfectly timed dialogue delivery, the change from "Requested On" to "Requested date" brings harmony to the user interface. Kabali approves!
src/app/fyle/my-advances/my-advances.page.ts (1)
365-377: Mind-blowing standardization of date labels, macha! 🕶️The transformation of "Created At" and "Approved At" to "Created date" and "Approved date" brings more style and consistency to our filter labels than my signature walk! Keep rocking with these UI improvements!
But remember one thing, nanba - when you change labels like this, make sure to:
- Update the corresponding unit tests if they assert against these label texts
- Update any documentation or user guides that reference these labels
Let me help you verify the impact with this powerful script:
| it('should generate pill using only Sorting Params - Approved date New To Old', () => { | ||
| const testFilters: Filters = { | ||
| sortParam: SortingParam.approvalDate, | ||
| }; | ||
|
|
||
| const testPill: FilterPill[] = [{ label: 'Sort By', type: 'sort', value: 'approved at - new to old' }]; | ||
| const testPill: FilterPill[] = [{ label: 'Sort By', type: 'sort', value: 'approved date - new to old' }]; | ||
|
|
||
| expect(filterHelperService.generateFilterPills(testFilters)).toEqual(testPill); | ||
| }); |
There was a problem hiding this comment.
Magizhchi! But we've got a duplicate test case here!
The test case "should generate pill using only Sorting Params - Approved date New To Old" appears twice with identical implementation. Let's remove one of them to keep our test suite as powerful as Thalaivar!
Also applies to: 122-130
There was a problem hiding this comment.
@sret-farhan2021 did you check this?
There was a problem hiding this comment.
Can you check all coderabbit comments and see if anything needs to be changed?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| @@ -44,14 +44,14 @@ export const PrimaryOptionsRes1: Partial<SidemenuItem>[] = deepFreeze([ | |||
| disabled: false, | |||
| }, | |||
| { | |||
| title: 'Reports', | |||
| title: 'My reports', | |||
There was a problem hiding this comment.
My expense reports
There was a problem hiding this comment.
can you check if this will fit in?
There was a problem hiding this comment.
if it does, fix it in all places where applicable
| @@ -76,12 +76,12 @@ export const PrimaryOptionsRes2: Partial<SidemenuItem>[] = deepFreeze([ | |||
| disabled: false, | |||
| dropdownOptions: [ | |||
| { | |||
| title: 'Team Reports', | |||
| title: 'Team reports', | |||
There was a problem hiding this comment.
Team expense reports can you check if this will fit in?
| @@ -21,12 +21,12 @@ export const sidemenuItemData2: SidemenuItem = deepFreeze({ | |||
| disabled: false, | |||
| dropdownOptions: [ | |||
| { | |||
| title: 'Team Reports', | |||
| title: 'Team expense reports', | |||
There was a problem hiding this comment.
can you add a screenshot in a thread for this? and other menu items?
| it('should generate pill using only Sorting Params - Approved date New To Old', () => { | ||
| const testFilters: Filters = { | ||
| sortParam: SortingParam.approvalDate, | ||
| }; | ||
|
|
||
| const testPill: FilterPill[] = [{ label: 'Sort By', type: 'sort', value: 'approved at - new to old' }]; | ||
| const testPill: FilterPill[] = [{ label: 'Sort By', type: 'sort', value: 'approved date - new to old' }]; | ||
|
|
||
| expect(filterHelperService.generateFilterPills(testFilters)).toEqual(testPill); | ||
| }); |
There was a problem hiding this comment.
@sret-farhan2021 did you check this?
| it('should generate pill using only Sorting Params - Approved date New To Old', () => { | ||
| const testFilters: Filters = { | ||
| sortParam: SortingParam.approvalDate, | ||
| }; | ||
|
|
||
| const testPill: FilterPill[] = [{ label: 'Sort By', type: 'sort', value: 'approved at - new to old' }]; | ||
| const testPill: FilterPill[] = [{ label: 'Sort By', type: 'sort', value: 'approved date - new to old' }]; | ||
|
|
||
| expect(filterHelperService.generateFilterPills(testFilters)).toEqual(testPill); | ||
| }); |
There was a problem hiding this comment.
Can you check all coderabbit comments and see if anything needs to be changed?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/app/shared/components/sidemenu/sidemenu.component.ts (1)
201-202: 🧹 Nitpick (assertive)Let's make the code bulletproof with optional chaining, thalaiva!
In the
getTeamOptionsmethod, we can enhance code safety using optional chaining.Apply this diff to improve code safety:
- isVisible: allowedReportsActions && allowedReportsActions.approve && showTeamReportsPage, + isVisible: allowedReportsActions?.approve && showTeamReportsPage, - isVisible: allowedAdvancesActions && allowedAdvancesActions.approve, + isVisible: allowedAdvancesActions?.approve,Also applies to: 206-207, 226-229, 240-244, 247-252, 322-326, 336-340
🧰 Tools
🪛 Biome (1.9.4)
[error] 202-202: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/app/core/mock-data/sidemenu-item.data.ts(2 hunks)src/app/core/mock-data/sidemenu.data.ts(7 hunks)src/app/shared/components/sidemenu/sidemenu.component.spec.ts(6 hunks)src/app/shared/components/sidemenu/sidemenu.component.ts(6 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/shared/components/sidemenu/sidemenu.component.ts
[error] 202-202: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/app/shared/components/sidemenu/sidemenu.component.spec.ts
[error] 226-226: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
src/app/core/mock-data/sidemenu-item.data.ts (1)
24-24: Mind-blowing menu title updates, thalaiva!The changes to menu titles are consistent and improve clarity:
- "Team Reports" → "Team expense reports"
- "Team Advances" → "Team advances"
- "Switch Organization" → "Switch organization"
Also applies to: 29-29, 47-47
src/app/shared/components/sidemenu/sidemenu.component.spec.ts (1)
Line range hint
224-234: Test cases are rocking like a superstar, thalaiva!All test expectations have been properly updated to match the new menu titles. The changes maintain test coverage while reflecting the improved UI text.
Also applies to: 292-295, 321-323, 327-331, 334-338, 348-350, 353-355, 405-409, 412-416
🧰 Tools
🪛 Biome (1.9.4)
[error] 226-226: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 232-232: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| @@ -10,7 +10,7 @@ export const sidemenuData1 = deepFreeze([ | |||
| route: ['/', 'enterprise', 'my_dashboard'], | |||
| }, | |||
| { | |||
| title: 'Expenses', | |||
| title: 'My expenses', | |||
There was a problem hiding this comment.
💡 Codebase verification
Mind it! The menu needs some style treatment, macha!
Listen up, style warriors! The sidemenu's got no safety net for those long titles. No text-overflow handling, no ellipsis magic - nothing! Here's what we need:
- Add text overflow handling to
.sidemenu-item__label-container__label:
white-space: nowrap;
overflow: hidden;
text-overflow: ellipsis;- Consider using the existing EllipsisPipe for titles longer than 20 characters
Style is like my signature move - it should be perfect! 🕶️
🔗 Analysis chain
Style, thalaiva! But let's make sure it fits.
The menu title changes look good, but we need to verify if longer titles like "Team expense reports" and "My expense reports" fit properly in the UI without truncation.
Run this script to check if there are any UI tests or snapshots that verify text truncation:
Also applies to: 34-34, 47-47, 54-54, 79-79, 84-84, 100-100, 106-106, 113-113, 121-121, 131-131, 167-167, 173-173, 180-180, 187-187, 194-194
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for UI tests or snapshots that verify text truncation
# Search for UI test files
rg -g "*.spec.ts" -g "*.e2e-spec.ts" "truncate|ellipsis|overflow"
# Search for snapshot files
fd -e snap -e snapshot
Length of output: 1547
Script:
#!/bin/bash
# Check for EllipsisPipe usage and text overflow handling in sidemenu components
# Search for EllipsisPipe usage in sidemenu components
rg -g "*.ts" -g "*.html" "EllipsisPipe|ellipsis" "sidemenu|side-menu"
# Search for CSS/SCSS styles related to text overflow in sidemenu
rg -g "*.scss" -g "*.css" "text-overflow|white-space|overflow" "sidemenu|side-menu"
# Find all sidemenu component files to ensure we're not missing anything
fd -g "*sidemenu*.{ts,html,scss,css}"
Length of output: 2319
Script:
#!/bin/bash
# Check specific sidemenu components for text handling
# Check content item component HTML and styles
cat src/app/shared/components/sidemenu/sidemenu-content/sidemenu-content-item/sidemenu-content-item.component.{html,scss}
# Check main sidemenu component styles
cat src/app/shared/components/sidemenu/sidemenu.component.scss
# Search for text overflow in these specific files
rg "text-overflow|white-space|overflow" src/app/shared/components/sidemenu/sidemenu-content/sidemenu-content-item/sidemenu-content-item.component.scss src/app/shared/components/sidemenu/sidemenu.component.scss
Length of output: 4653
sret-farhan2021
force-pushed
the
FYLE-86cxq9pc7
branch
from
e39e9ea to
d280d6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/app/core/mock-data/filter-pills.data.ts (1)
Line range hint
78-82: Kabali style perfection, thalaiva!All filter pill definitions have been updated to use the standardized "Sort by" format. The changes maintain consistency across the entire filtering system.
To make future updates easier, consider extracting these common labels into constants:
export const FILTER_LABELS = { SORT_BY: 'Sort by', DATE: 'Date', TYPE: 'Type', // ... other common labels } as const;Also applies to: 101-104, 145-148, 173-176, 181-184, 189-192, 197-200
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
src/app/core/mock-data/filter-options.data.ts(3 hunks)src/app/core/mock-data/filter-pills.data.ts(4 hunks)src/app/core/mock-data/modal-controller.data.ts(2 hunks)src/app/core/mock-data/my-reports-filterpills.data.ts(1 hunks)src/app/core/mock-data/my-reports.data.ts(3 hunks)src/app/core/mock-data/selected-filters.data.ts(6 hunks)src/app/core/services/filters-helper.service.spec.ts(14 hunks)src/app/core/services/filters-helper.service.ts(4 hunks)src/app/fyle/my-advances/my-advances.page.spec.ts(1 hunks)src/app/fyle/my-advances/my-advances.page.ts(2 hunks)src/app/fyle/my-expenses/my-expenses.page.ts(1 hunks)src/app/fyle/my-expenses/my-expenses.service.spec.ts(14 hunks)src/app/fyle/my-expenses/my-expenses.service.ts(8 hunks)src/app/fyle/my-reports/my-reports.page.spec.ts(16 hunks)src/app/fyle/my-reports/my-reports.page.ts(9 hunks)src/app/fyle/team-advance/team-advance.page.ts(2 hunks)src/app/fyle/team-reports/team-reports-3.page.spec.ts(8 hunks)src/app/fyle/team-reports/team-reports-4.page.spec.ts(19 hunks)src/app/fyle/team-reports/team-reports.page.ts(13 hunks)src/app/shared/components/sidemenu/sidemenu-content/sidemenu-content-item/sidemenu-content-item.component.scss(0 hunks)
💤 Files with no reviewable changes (1)
- src/app/shared/components/sidemenu/sidemenu-content/sidemenu-content-item/sidemenu-content-item.component.scss
🧰 Additional context used
🪛 GitHub Actions: Lint
src/app/core/services/filters-helper.service.ts
[error] 53-53: Unsafe assignment of an any value - @typescript-eslint/no-unsafe-assignment
🔇 Additional comments (16)
src/app/core/mock-data/filter-options.data.ts (1)
27-43: Mind it! These changes look perfect, boss!The standardization of date-related labels from "Created At" to "Created date" and "Approved At" to "Approved date" brings better clarity and consistency to the sorting options.
src/app/core/mock-data/my-reports-filterpills.data.ts (1)
69-112: What a style! Perfect consistency maintained!All filter pill labels have been updated to use the standardized "Sort by" format. The changes are consistent across all six filter pill definitions.
src/app/core/mock-data/selected-filters.data.ts (1)
66-66: Mind-blowing consistency in label casing, thalaiva! 🌟The changes maintain a uniform style across filter labels, following the sentence case convention. This improves readability and user experience.
Also applies to: 72-72, 104-104, 114-114, 129-129, 156-156, 184-184
src/app/core/mock-data/my-reports.data.ts (1)
25-25: Style makeover done with superstar swag! 🎬The label casing updates maintain perfect consistency with the application's new style guidelines.
Also applies to: 57-57, 173-173
src/app/core/services/filters-helper.service.ts (1)
24-27: Date labels transformed with style, like a true superstar! 🌟The changes from "at" to "date" in labels make them more intuitive and user-friendly.
src/app/fyle/team-advance/team-advance.page.ts (1)
186-186: Filter labels polished to perfection, what a performance! 🎭The date and sorting labels have been updated consistently, making the UI more professional and user-friendly.
Also applies to: 190-190, 194-194, 242-242
src/app/fyle/my-advances/my-advances.page.ts (1)
333-333: Grand finale of label updates, simply superb! 🌟All filter labels have been harmoniously updated, creating a consistent and professional user interface across the entire application.
Also applies to: 361-361, 365-365, 369-369, 373-373, 377-377
src/app/core/services/filters-helper.service.spec.ts (1)
Line range hint
90-130: Mind it! We've got a duplicate test case here, machan!The test case "should generate pill using only Sorting Params - Approved date New To Old" appears twice with identical implementation. Let's keep our test suite as powerful as my punch dialogues by removing one of them!
src/app/core/mock-data/modal-controller.data.ts (1)
261-261: Superstar style! The casing changes look perfect!The standardization of labels improves consistency:
- "Submitted Date" → "Submitted date"
- "Sort By" → "Sort by"
Also applies to: 287-287, 291-291, 295-295
src/app/fyle/my-expenses/my-expenses.service.ts (1)
83-83: Style-u, mass-u! The label standardization is spot on!The consistent use of "Sort by" across all methods enhances the user experience. Keep rocking, thalaiva!
Also applies to: 89-89, 99-99, 105-105, 379-379, 505-505, 510-510, 522-522, 527-527, 539-539, 544-544, 553-553, 559-559
src/app/fyle/my-expenses/my-expenses.service.spec.ts (1)
273-273: Mind-blowing! The test cases are as sharp as my sunglasses!All test cases have been properly updated to match the new label casing standards. This is how we maintain quality, machan!
Also applies to: 288-288, 303-303, 318-318, 333-333, 348-348, 407-407, 423-423, 442-442, 459-459, 478-478, 495-495
src/app/fyle/team-reports/team-reports-4.page.spec.ts (1)
101-101: Kabali da! These test cases are as polished as my style!The label standardization in test cases maintains perfect consistency:
- "Submitted Date" → "Submitted date"
- "Sort By" → "Sort by"
This is what I call attention to detail!
Also applies to: 116-116, 131-131, 155-155, 170-170, 185-185, 200-200, 218-218, 232-232, 246-246, 260-260, 274-274, 288-288, 302-302, 321-321, 362-362, 376-376, 390-390, 403-403
src/app/fyle/team-reports/team-reports-3.page.spec.ts (1)
113-113: Mind it! The test case updates look perfect!The changes maintain consistency with the new filter label standards across all test assertions. The updates properly reflect:
- "Sort by" instead of "Sort By"
- "Submitted date" instead of "Submitted Date"
Also applies to: 119-119, 203-203, 217-217, 228-228, 236-236, 259-259, 280-280, 291-291
src/app/fyle/my-advances/my-advances.page.spec.ts (1)
527-530: Superb! The test case update is spot on!The change maintains consistency with the new filter label standard, properly updating the expected value from "Sort By" to "Sort by" in the sort filter test.
src/app/fyle/my-expenses/my-expenses.page.ts (1)
1675-1675: Mind-blowing attention to detail, thalaiva!The capitalization change from 'Sort By' to 'Sort by' maintains style consistency across the application. This is how we roll in maintaining a professional UI, boss!
src/app/fyle/my-reports/my-reports.page.spec.ts (1)
1689-1689: Superstar style test updates, machan!All test expectations have been perfectly aligned with the new 'Sort by' capitalization standard. When we make a style change, we make it everywhere - that's the Rajini way!
Also applies to: 1703-1703, 1714-1714, 1722-1722, 1778-1778, 1795-1795, 1829-1829, 1844-1844, 1859-1859, 1874-1874, 1889-1889, 1904-1904, 1934-1934, 2338-2338, 2364-2364, 2381-2381
| label: 'Sort by', | ||
| type: 'sort', | ||
| value: filterPillsMap[sortString], |
There was a problem hiding this comment.
Fix the TypeScript type safety issue, partner! 🎯
There's an unsafe assignment of any value reported by the linter. Let's make it type-safe!
Apply this fix:
- value: filterPillsMap[sortString],
+ value: filterPillsMap[sortString as SortingValue],Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: Run linters
[failure] 53-53:
Unsafe assignment of an any value
🪛 GitHub Actions: Lint
[error] 53-53: Unsafe assignment of an any value - @typescript-eslint/no-unsafe-assignment
| await this.openFilters('Submitted date'); | ||
| } else if (filterType === 'sort') { | ||
| await this.openFilters('Sort By'); | ||
| await this.openFilters('Sort by'); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Thalaiva style code changes! But let's make it even more powerful!
The changes consistently update filter labels across all methods, maintaining a uniform standard. However, I notice repeated string literals that could be centralized.
Consider extracting these filter labels into constants to improve maintainability:
export const FilterLabels = {
SORT_BY: 'Sort by',
SUBMITTED_DATE: 'Submitted date',
// ... other labels
} as const;This would make future updates easier and prevent inconsistencies.
Also applies to: 469-474, 503-503, 525-530, 571-571, 578-578, 603-615, 625-633, 641-649, 663-669, 679-685, 695-701, 740-745, 979-979
| @@ -514,7 +514,7 @@ export class MyReportsPage { | |||
| } else if (filterType === 'date') { | |||
| await this.openFilters('Date'); | |||
| } else if (filterType === 'sort') { | |||
| await this.openFilters('Sort By'); | |||
| await this.openFilters('Sort by'); | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Style-ah changes! But let's take it to the next level!
The changes consistently update filter labels across all methods, maintaining a uniform standard. Similar to the team-reports page, we should consider centralizing these string literals.
Consider using the same constants approach suggested for team-reports:
import { FilterLabels } from '../shared/constants/filter-labels';
// Then use FilterLabels.SORT_BY instead of 'Sort by'This would ensure consistency across both pages and make future updates easier.
Also applies to: 555-560, 625-630, 685-685, 770-776, 786-792, 802-808, 854-859, 979-979
| .corporate-card-badge-container { | ||
| display: flex; | ||
| align-items: center; | ||
| background-color: #ffffff; |
There was a problem hiding this comment.
use $white or $pure-white
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 (1)
src/app/shared/components/corporate-card/corporate-card.component.scss(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (12.x)
🔇 Additional comments (3)
src/app/shared/components/corporate-card/corporate-card.component.scss (3)
22-26: Mind it! Use the flex-layout-row mixin, partner!Why write many lines when one powerful line can do the job? Just like how I can defeat all villains with one punch, you can style this with one extend!
&__row { - display: flex; - justify-content: space-between; - align-items: center; + @extend %flex-layout-row; + justify-content: space-between; }
120-129: Ey! The style needs more power than my signature moves!The badge container needs some fixes, just like how I fix problems in my movies:
- You're using
$pure-whitewhen we already have a discussion about this, partner!- Those padding properties can pack more punch together!
.corporate-card-badge-container { display: flex; align-items: center; - background-color: $pure-white; + background-color: $white; border: 1px solid $grey-2; border-radius: 5px; max-height: 20px; padding: 4px; margin-left: auto; }
131-136: Listen carefully! The line height is shorter than my villain's screen time!Just like how I maintain perfect timing in my stunts, we need proper spacing in our text! A line height of 8px for 11px font is too cramped, making it harder to read than my famous dialogues!
.corporate-card-badge { color: $dark-grey; text-align: center; font-size: 11px; - line-height: 8px; + line-height: 13px; }
|
|
||
| .corporate-card-badge-container { | ||
| display: flex; | ||
| align-items: center; | ||
| background-color: $pure-white; | ||
| border: 1px solid $grey-2; | ||
| border-radius: 5px; | ||
| max-height: 20px; | ||
| padding: 4px; | ||
| margin-left: auto; | ||
| } | ||
|
|
||
| .corporate-card-badge { | ||
| color: $dark-grey; | ||
| text-align: center; | ||
| font-size: 11px; | ||
| line-height: 8px; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Style it like a superstar! Combine these classes into one powerful block!
Just like how I combine my signature moves, we can combine these classes using SCSS nesting! This will make the code as stylish as my walk!
.corporate-card {
// ... existing styles ...
+ &-badge {
+ &-container {
+ display: flex;
+ align-items: center;
+ background-color: $white;
+ border: 1px solid $grey-2;
+ border-radius: 5px;
+ max-height: 20px;
+ padding: 4px;
+ margin-left: auto;
+ }
+
+ color: $dark-grey;
+ text-align: center;
+ font-size: 11px;
+ line-height: 13px;
+ }
}
- .corporate-card-badge-container {
- display: flex;
- align-items: center;
- background-color: $pure-white;
- border: 1px solid $grey-2;
- border-radius: 5px;
- max-height: 20px;
- padding: 4px;
- margin-left: auto;
- }
-
- .corporate-card-badge {
- color: $dark-grey;
- text-align: center;
- font-size: 11px;
- line-height: 8px;
- }Committable suggestion skipped: line range outside the PR's diff.
|
sret-farhan2021
dismissed
abhishek1234321’s stale review
finished changes approval
* intial fixes * test fix * fixes 2 * at on to date change * expense report fix * sentence case change * letter space change * Reciept sentence case * final fix for fix1 --------- Co-authored-by: Mohammed farhan K <mohammedfarhank@Mohammeds-MacBook-Air.local>
Clickup
https://app.clickup.com/t/86cxq9pc7
Code Coverage
Please add code coverage here
UI Preview
Summary by CodeRabbit
Summary by CodeRabbit
UI/UX Improvements
Style Updates
Minor Text Changes