feat: Milestone 6: Dashboard > Tasks display exact amount #3361
Conversation
WalkthroughIn a spectacular display of precision, this pull request transforms the monetary landscape of our application! The changes sweep across multiple files, focusing on currency representation and task-related mock data. We've upgraded our currency handling with the Changes
Suggested Reviewers
Possibly Related PRs
Poem
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: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/app/core/mock-data/task.data.ts(7 hunks)src/app/core/services/tasks.service.spec.ts(23 hunks)src/app/core/services/tasks.service.ts(8 hunks)src/app/fyle/dashboard/tasks/tasks-card/tasks-card.component.html(2 hunks)src/app/fyle/dashboard/tasks/tasks-card/tasks-card.component.scss(4 hunks)src/app/fyle/dashboard/tasks/tasks-card/tasks-card.component.spec.ts(2 hunks)src/app/shared/shared.module.ts(1 hunks)
🔇 Additional comments (3)
src/app/fyle/dashboard/tasks/tasks-card/tasks-card.component.scss (1)
11-11: 🧹 Nitpick (assertive)
Mind it! Font sizes might be too small for comfortable reading, macha!
Reducing all icon sizes to 20px uniformly might affect visibility. Consider maintaining slightly larger sizes for critical icons like warnings (24px).
Let me check the accessibility guidelines for you:
Also applies to: 18-18, 28-28, 35-35
src/app/core/services/tasks.service.spec.ts (2)
5-5: Mind it! The test setup is perfect, just like my style!
The ExactCurrencyPipe is properly imported and configured in the test environment with all the necessary spy objects and providers.
Also applies to: 84-84, 106-106, 125-128, 190-190
209-214: Kabali says these currency pipe changes are spot on!
The test cases have been systematically updated to use ExactCurrencyPipe with proper parameters and expectations. The changes maintain consistent test coverage across all scenarios.
Also applies to: 225-230, 260-265, 280-285, 302-307, 319-324, 343-348, 375-380, 950-959, 1021-1033
|
|
||
| &--cta-row { | ||
| padding-top: 17px; | ||
| padding-top: 11px; | ||
| max-width: fit-content; | ||
| display: flex; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Style! The CTA row needs some breathing space, boss!
The padding adjustments make the layout too tight. Consider:
- Maintaining some minimal padding for touch targets
- Ensuring proper spacing between icon and text
&--cta-row {
padding-top: 11px;
max-width: fit-content;
display: flex;
+ gap: 8px;
&-icon {
color: $black;
font-size: 14px;
- padding: 0;
+ padding: 0 4px;
}
}Also applies to: 50-52
| @@ -2,7 +2,7 @@ | |||
| <div class="task--card"> | |||
| <ion-grid> | |||
| <ion-row class="task--row" (click)="taskCtaClicked(task)" [ngClass]="{ 'pb-0': showReportAutoSubmissionInfo }"> | |||
| <ion-col size="1" class="task--icon-col"> | |||
| <ion-col size="0.75" class="task--icon-col"> | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
The layout structure needs more power! Let me show you how!
The current structure with nested rows and columns could be simplified:
-<ion-col size="0.75" class="task--icon-col">
+<ion-col size="1" class="task--icon-col">
<ion-row>
- <ion-col class="task--icon-col">
<ion-icon
class="task--icon"
[ngClass]="..."
[src]="..."
></ion-icon>
- </ion-col>
</ion-row>
</ion-col>Also applies to: 22-28
| {{ task.subheader }} | ||
| </ion-col> | ||
| </ion-row> | ||
| <ion-row class="ion-justify-content-between task--cta-row"> | ||
| <ion-col size="11" *ngFor="let cta of task.ctas" class="task--cta"> | ||
| <ion-row class="ion-justify-contant-start task--cta-row"> |
There was a problem hiding this comment.
Ey! There's a typo in your class name that even my signature style can't fix!
ion-justify-contant-start should be ion-justify-content-start. This typo could break your layout, partner!
-<ion-row class="ion-justify-contant-start task--cta-row">
+<ion-row class="ion-justify-content-start task--cta-row">📝 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.
| <ion-row class="ion-justify-contant-start task--cta-row"> | |
| <ion-row class="ion-justify-content-start task--cta-row"> |
| it('should display the correct header and amount for the first task', () => { | ||
| const taskHeader = getElementBySelector(fixture, '.task--header'); | ||
| expect(getTextContent(taskHeader)).toContain('Complete'); | ||
|
|
||
| const taskSubheader = getElementBySelector(fixture, '.task--subheader'); | ||
| expect(getTextContent(taskSubheader)).toContain('13 expenses worth ₹142.26K can be added to a report'); | ||
| const taskAmount = getElementBySelector(fixture, '.task--amount'); | ||
| expect(getTextContent(taskAmount)).toContain('14200.26'); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Your tests are strong like me, but let's make them stronger!
Add more test cases to verify:
- Currency symbol display
- Different amount formats
- Hidden amount scenarios
it('should display currency symbol correctly', () => {
component.currencySymbol$ = of('$');
fixture.detectChanges();
const amountElement = getElementBySelector(fixture, '.task--amount-currency');
expect(getTextContent(amountElement)).toContain('$');
});
it('should handle hidden amounts correctly', () => {
component.task.hideAmount = true;
fixture.detectChanges();
const amountElement = getElementBySelector(fixture, '.task--amount');
expect(amountElement).toBeNull();
});| @@ -18,10 +18,10 @@ export const draftExpenseTaskSample = deepFreeze({ | |||
| }); | |||
|
|
|||
| export const draftExpenseTaskSample2 = deepFreeze({ | |||
| amount: '132.57B', | |||
| amount: '76234.47', | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mind-blowing changes, but let's make the test data more realistic, macha!
The mock data has some concerns that need your attention:
- Some amounts are unrealistically large (e.g. ₹5,177,243,929.65)
- Decimal precision is inconsistent across different samples
Consider:
- Using more realistic amount ranges that match production scenarios
- Maintaining consistent decimal places (preferably 2) across all amounts
- Using round numbers where possible to improve readability of test data
Also applies to: 24-24, 49-49, 52-52, 63-63, 66-66, 77-77, 80-80, 105-105, 108-108, 119-119, 122-122, 133-133, 136-136
| @@ -418,6 +418,6 @@ import { ExactCurrencyPipe } from './pipes/exact-currency.pipe'; | |||
| DashboardOptInComponent, | |||
| MobileNumberCardComponent, | |||
| ], | |||
| providers: [DecimalPipe, DatePipe, HumanizeCurrencyPipe, ImagePicker, FyCurrencyPipe, ReportState], | |||
| providers: [DecimalPipe, DatePipe, HumanizeCurrencyPipe, ImagePicker, FyCurrencyPipe, ReportState, ExactCurrencyPipe], | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Style-ah deprecate the old pipe, thalaiva!
Since we're moving to exact currency display, consider deprecating HumanizeCurrencyPipe to prevent confusion and maintain consistency.
- Add
@Deprecateddecorator toHumanizeCurrencyPipe - Add documentation comment explaining the migration to
ExactCurrencyPipe - Plan for removal in future versions
| @@ -43,6 +44,7 @@ export class TasksService { | |||
| constructor( | |||
| private reportService: ReportService, | |||
| private humanizeCurrency: HumanizeCurrencyPipe, | |||
| private exactCurrency: ExactCurrencyPipe, | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Punch dialogue: Let's add some documentation, boss!
The ExactCurrencyPipe injection lacks documentation about its purpose and configuration.
Add JSDoc comment explaining the pipe's usage:
/**
* ExactCurrencyPipe for precise currency formatting
* Always used with skipSymbol: true as currency symbol is handled separately
*/
private exactCurrency: ExactCurrencyPipe,| amount: this.exactCurrency.transform({ | ||
| value: aggregate.total_amount, | ||
| currencyCode: homeCurrency, | ||
| skipSymbol: true, | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mind it! Let's reduce the code duplication, thalaiva!
The transform options object is repeated across multiple methods with the same configuration.
Create a private method to handle the currency transformation:
private transformCurrency(value: number, currencyCode: string): string {
return this.exactCurrency.transform({
value,
currencyCode,
skipSymbol: true
});
}Then use it across all methods:
-amount: this.exactCurrency.transform({
- value: aggregate.totalAmount,
- currencyCode: homeCurrency,
- skipSymbol: true,
-}),
+amount: this.transformCurrency(aggregate.totalAmount, homeCurrency),Also applies to: 734-738, 760-764, 789-793, 821-825, 852-856
| @@ -684,14 +686,18 @@ | |||
| } | |||
|
|
|||
| getAmountString(amount: number, currency: string): string { | |||
| return amount > 0 ? ` worth ${this.humanizeCurrency.transform(amount, currency)} ` : ''; | |||
| return amount > 0 ? ` worth ${this.exactCurrency.transform({ value: amount, currencyCode: currency })} ` : ''; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Superstar move on the getAmountString method!
The implementation is clean but differs from other transform calls by not using skipSymbol: true. This inconsistency could lead to formatting differences.
Add skipSymbol: true to match other transform calls:
- return amount > 0 ? ` worth ${this.exactCurrency.transform({ value: amount, currencyCode: currency })} ` : '';
+ return amount > 0 ? ` worth ${this.exactCurrency.transform({ value: amount, currencyCode: currency, skipSymbol: true })} ` : '';📝 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.
| return amount > 0 ? ` worth ${this.exactCurrency.transform({ value: amount, currencyCode: currency })} ` : ''; | |
| return amount > 0 ? ` worth ${this.exactCurrency.transform({ value: amount, currencyCode: currency, skipSymbol: true })} ` : ''; |
| exactCurrencyPipe.transform | ||
| .withArgs({ value: totalCount, currencyCode: homeCurrency, skipSymbol: true }) | ||
| .and.returnValue('339.00'); | ||
| exactCurrencyPipe.transform.withArgs({ value: totalCount, currencyCode: homeCurrency }).and.returnValue('₹339.00'); |
There was a problem hiding this comment.
Aiyyo! There's a small confusion in the test expectations, machan!
The test is using the wrong value for currency transformation. It's using totalCount instead of a proper amount value, which could lead to incorrect test results.
Apply this fix:
- const totalCount = incompleteStats.data.count;
- exactCurrencyPipe.transform
- .withArgs({ value: totalCount, currencyCode: homeCurrency, skipSymbol: true })
- .and.returnValue('339.00');
- exactCurrencyPipe.transform.withArgs({ value: totalCount, currencyCode: homeCurrency }).and.returnValue('₹339.00');
+ const totalAmount = incompleteStats.data.total_amount;
+ exactCurrencyPipe.transform
+ .withArgs({ value: totalAmount, currencyCode: homeCurrency, skipSymbol: true })
+ .and.returnValue('339.00');
+ exactCurrencyPipe.transform.withArgs({ value: totalAmount, currencyCode: homeCurrency }).and.returnValue('₹339.00');Committable suggestion skipped: line range outside the PR's diff.
|
Important
This is the feature branch.
This PR contains all the approved child PRs ✅
Clickup
clickup link
Summary by CodeRabbit
New Features
ExactCurrencyPipefor more precise currency formatting across various components and services.tasks-cardcomponent for improved user experience.Bug Fixes
Style
tasks-cardcomponent, including font sizes and padding for a more consistent look.Documentation
ExactCurrencyPipeto theSharedModulefor broader application use.