feat: Milestone 7: Dashboard > Home display exact amount #3376
Conversation
WalkthroughIn a spectacular display of code transformation, this pull request revolutionizes the currency display logic across multiple components! The changes introduce a new Changes
Suggested Reviewers
Possibly Related PRs
Suggested Labels
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
src/app/fyle/dashboard/stat-badge/stat-badge.component.html(3 hunks)src/app/fyle/dashboard/stat-badge/stat-badge.component.scss(1 hunks)src/app/fyle/dashboard/stat-badge/stat-badge.component.spec.ts(2 hunks)src/app/fyle/dashboard/stat-badge/stat-badge.component.ts(1 hunks)src/app/fyle/dashboard/stats/stats.component.scss(2 hunks)src/app/shared/components/spent-cards/card-detail/card-detail.component.html(2 hunks)src/app/shared/components/spent-cards/card-detail/card-detail.component.scss(1 hunks)src/app/shared/components/spent-cards/card-detail/card-detail.component.spec.ts(2 hunks)src/app/shared/components/spent-cards/card-detail/card-detail.component.ts(1 hunks)
🔇 Additional comments (9)
src/app/fyle/dashboard/stats/stats.component.scss (1)
23-23: Mind it! This color change is a blockbuster decision!
Switching to $black-light improves the contrast ratio, making our stats header as powerful as my signature sunglasses, macha! Your users' eyes will thank you.
src/app/shared/components/spent-cards/card-detail/card-detail.component.scss (2)
44-44: Mind-blowing vertical alignment, partner!
Adding align-items: center is like my signature move - perfectly balanced, as all things should be! This change ensures your content stays centered vertically, making it as sharp as a well-choreographed action sequence.
51-56: New class enters the scene with style, thalaiva! 🎬
The .stats--ccc-classified-count class makes a grand entrance with perfect typography settings! The combination of 500 weight and 16px size creates that perfect balance, just like how I balance action and style!
src/app/fyle/dashboard/stat-badge/stat-badge.component.ts (1)
Line range hint 31-38: Style! The return type addition is as perfect as my timing!
The void return type makes the method signature crystal clear. Keep it up!
src/app/fyle/dashboard/stat-badge/stat-badge.component.scss (1)
79-80: 🧹 Nitpick (assertive)
Listen up! Small fonts need big consideration!
The 12px font size might be too small for some users to read comfortably. According to WCAG guidelines, we should ensure sufficient contrast and readability.
Let's check other components' font sizes:
Consider:
- Using relative units (rem) instead of pixels
- Setting a minimum font size of 14px for better readability
- Adding a CSS variable for consistent font sizing across components
Also applies to: 84-84
✅ Verification successful
The previous command failed because the SCSS file type wasn't recognized. Let's try a more robust approach to find font-size declarations across the codebase. This will help us verify the consistency of font sizes and accessibility standards.
Note: If this doesn't yield results, we might need to try different approaches or recommend manual verification.
Mind it! The 12px font size is perfectly fine, boss!
After analyzing the entire codebase's font sizes like a true style warrior, I can see that 12px is a commonly used font size throughout the application for secondary text and labels. Here's what I found, thalaiva:
- The codebase consistently uses 12px for metadata, labels, and supporting text
- Important content uses larger sizes (14-20px)
- The stat badge component follows this established pattern with other similar components
The 12px font size meets accessibility standards when used for supplementary content, especially with proper contrast ratio that this component maintains with its black color.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all SCSS files and check for font-size declarations
rg --type scss "font-size:" -B 1 -A 1
Length of output: 66
Script:
#!/bin/bash
# Search for font-size declarations in all files (using ripgrep without file type filter)
rg "font-size:" -B 1 -A 1
# Also search specifically in .scss and .css files
find . -type f \( -name "*.scss" -o -name "*.css" \) -exec grep -A 1 -B 1 "font-size:" {} \;
Length of output: 75313
src/app/shared/components/spent-cards/card-detail/card-detail.component.ts (1)
19-20: Same screen detection power-up needed here!
Just like in StatBadgeComponent, we need to make the screen detection dynamic.
Please refer to the previous comment about adding resize listener in StatBadgeComponent. The same solution applies here!
src/app/fyle/dashboard/stat-badge/stat-badge.component.html (1)
5-5: Mind the grid balance, macha!
The column size adjustment from 4/8 to 3/9 makes the badge more compact. But like my punch dialogues, timing is everything - verify this layout works well across all screen sizes!
Also applies to: 26-26
✅ Verification successful
Let me search for more evidence about the layout structure, macha!
Let me search for more evidence about the grid system, macha!
Mind the grid balance? No worries, macha! This layout is as solid as my style!
Looking at the component's SCSS file, I can see the grid system is well-structured with proper styling for both the container and columns. The layout uses fixed heights and proper spacing:
- Fixed height constraints (min/max of 64px)
- Proper padding and margins
- Responsive text handling with ellipsis and overflow control
- Zero padding grid system that allows precise control
The column size change from 4/8 to 3/9 is safe because:
- The count column has a fixed width element (30px circle)
- The text has proper overflow handling with
text-overflow: ellipsis - The grid uses
--ion-grid-padding: 0which prevents unwanted spacing issues
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for responsive layout tests
rg -l "TestBed.createComponent.*StatBadgeComponent.*" | xargs rg "viewport|screen|responsive"
Length of output: 95
Script:
#!/bin/bash
# Let's search for the component test file first
fd "stat-badge.component.spec"
# Then look for responsive design related files
fd "responsive|screen|viewport" --type f
# Check for any screen-related CSS classes in the component
rg -A 2 "stat-badge--count-col"
Length of output: 872
Script:
#!/bin/bash
# Search for ion-grid related patterns in the component
rg -A 3 "ion-grid|ion-row|ion-col" --type html
# Look for any media queries or responsive styles
rg "@media|screen" --type scss
Length of output: 97291
Script:
#!/bin/bash
# Let's check for any responsive styles in the component's SCSS file
fd "stat-badge.component.scss" --type f | xargs cat
# And look for any media queries in the parent component's styles
fd "dashboard.component.scss" --type f | xargs cat
# Check for any responsive test cases
rg "describe.*responsive|it.*responsive|test.*responsive" --type ts
Length of output: 1669
Script:
#!/bin/bash
# Look for any grid-related styles that might affect the layout
rg "stat-badge--count-col|ion-col" --type scss
# Check if there are any similar grid patterns in other components
rg "size=\"3\".*ion-col|size=\"9\".*ion-col" --type html
Length of output: 132
src/app/shared/components/spent-cards/card-detail/card-detail.component.spec.ts (2)
9-9: Mind it! The import statement is perfectly balanced, like my signature sunglasses! 😎
The ExactCurrencyPipe import follows the correct Angular conventions, maintaining style and power!
45-51: Thalaiva approves this TestBed configuration! 🌟
The declarations array is now more powerful than ever, with ExactCurrencyPipe joining forces with other pipes! But remember, with great power comes great responsibility - verify the pipe's behavior.
Let me check if we're testing this new pipe properly:
✅ Verification successful
Mind-blowing test coverage, machan! ExactCurrencyPipe is ready for action! 🌟
Listen here, my friend! The pipe's test file is a blockbuster hit with:
- 12 powerful test cases covering everything from positive to negative numbers
- Perfect handling of currency symbols and fraction digits
- Rock-solid edge cases like zero values and large numbers
The pipe is being used exactly where it should be in card-detail.component.html and stat-badge.component.html, working hand in hand with its companion pipes like a well-choreographed action sequence!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if ExactCurrencyPipe has proper test coverage
# Search for ExactCurrencyPipe tests
rg -l "describe.*ExactCurrencyPipe" --type ts
# Check if the pipe is being used in component templates
ast-grep --pattern 'exactCurrency'
Length of output: 131
Script:
#!/bin/bash
# Let's check the test coverage of ExactCurrencyPipe, Thalaiva style!
# First, let's see what's inside the pipe's test file
cat src/app/shared/pipes/exact-currency.pipe.spec.ts
# Then, let's see how it's being used in the card-detail component
rg -A 5 "exactCurrency" src/app/shared/components/spent-cards/card-detail/card-detail.component.ts
rg -A 5 "exactCurrency" src/app/shared/components/spent-cards/card-detail/card-detail.component.html
# Finally, let's check if there are any other components using this pipe
rg -l "exactCurrency" --type html
Length of output: 6470
| @@ -87,14 +87,12 @@ $ccc-bank-name: #c4cae8; | |||
| } | |||
|
|
|||
| &--report-stats-row { | |||
| padding-bottom: 18px; | |||
| padding-top: 18px; | |||
| padding: 24px 0 20px 0; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Style like a boss! But we can make it even more stylish!
The padding consolidation is clean, but why stop there? When two elements share the same style, they can share the same SCSS selector too!
Here's a super-punch refactor, style:
- &--report-stats-row {
- padding: 24px 0 20px 0;
- --ion-grid-column-padding: 0;
- }
-
- &--expenses-stats-row {
- padding: 24px 0 20px 0;
- --ion-grid-column-padding: 0;
- }
+ &--report-stats-row,
+ &--expenses-stats-row {
+ padding: 24px 0 20px 0;
+ --ion-grid-column-padding: 0;
+ }Also applies to: 95-95
| &--ccc-classified-amount { | ||
| font-weight: 500; | ||
| font-size: 18px; | ||
| font-size: 14px; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Font size reduction for better responsiveness, what a performance! 🌟
Reducing font sizes from 18px to 14px and 16px to 14px is like my famous punch dialogues - compact but powerful! This change aligns perfectly with the small screen detection mentioned in the summary.
However, mind-blowing suggestion coming up!
Consider using SCSS variables for these font sizes, making them as legendary as my movie titles:
+$font-size-regular: 14px;
+
.stats {
&--ccc-classified-amount {
font-weight: 500;
- font-size: 14px;
+ font-size: $font-size-regular;
}
&--ccc-stats-title {
- font-size: 14px;
+ font-size: $font-size-regular;
}
}Also applies to: 66-66
| align-items: center; | ||
|
|
||
| &__margin-top { | ||
| margin-top: 24px; | ||
| } | ||
| } | ||
|
|
||
| &--ccc-classified-count, | ||
| &--ccc-classified-count { | ||
| font-weight: 500; | ||
| font-size: 16px; | ||
| line-height: 28px; | ||
| color: $black; | ||
| } | ||
|
|
||
| &--ccc-classified-amount { | ||
| font-weight: 500; | ||
| font-size: 18px; | ||
| font-size: 14px; | ||
| line-height: 28px; | ||
| color: $black; | ||
| } | ||
|
|
||
| &--ccc-stats-title { | ||
| font-size: 16px; | ||
| font-size: 14px; | ||
| line-height: 20px; | ||
| color: $black-2; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
One more thing, superstar! Let's talk about screen sizes! 📱
The AI summary mentioned screen size detection, but these styles don't have media queries. Like how I handle different camera angles in my movies, we should handle different screen sizes with style!
Consider adding media queries for better responsive design:
@media screen and (max-width: 320px) {
.stats {
&--ccc-classified-amount,
&--ccc-stats-title {
font-size: 12px;
}
}
}| // To track if the screen is small (370px or below) | ||
| isSmallScreen = window.innerWidth <= 370; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mind-blowing! But let's make this screen detection more powerful, like my punch!
The screen size detection needs to be as dynamic as my action sequences! Currently, it only checks once during initialization.
Add this powerful resize listener, like this:
+ import { Component, Input, Output, EventEmitter, OnInit, OnDestroy } from '@angular/core';
+ import { Subject } from 'rxjs';
+ import { takeUntil } from 'rxjs/operators';
export class StatBadgeComponent implements OnInit, OnDestroy {
+ private destroy$: Subject<void> = new Subject();
isSmallScreen = window.innerWidth <= 370;
+
+ ngOnInit(): void {
+ fromEvent(window, 'resize')
+ .pipe(takeUntil(this.destroy$))
+ .subscribe(() => {
+ this.isSmallScreen = window.innerWidth <= 370;
+ });
+ }
+
+ ngOnDestroy(): void {
+ this.destroy$.next();
+ this.destroy$.complete();
+ }Committable suggestion skipped: line range outside the PR's diff.
| // To track if the screen is small (320px or below) | ||
| isSmallScreen = window.innerWidth <= 320; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Hold on! These breakpoints are dancing to different tunes!
I notice we're using different breakpoints:
- StatBadgeComponent: 370px
- CardDetailComponent: 320px
This inconsistency could create a confusing user experience, like a movie with mismatched scenes!
Let's make this epic by:
- Creating a shared constant for breakpoints:
// src/app/shared/constants/breakpoints.ts
export const SCREEN_BREAKPOINTS = {
EXTRA_SMALL: 320,
SMALL: 370,
// Add more as needed
} as const;- Creating a shared service for screen size detection:
// src/app/shared/services/screen-size.service.ts
@Injectable({
providedIn: 'root'
})
export class ScreenSizeService {
private screenSize$ = new BehaviorSubject<{
isExtraSmall: boolean;
isSmall: boolean;
}>({
isExtraSmall: window.innerWidth <= SCREEN_BREAKPOINTS.EXTRA_SMALL,
isSmall: window.innerWidth <= SCREEN_BREAKPOINTS.SMALL
});
constructor() {
fromEvent(window, 'resize')
.pipe(
debounceTime(100)
)
.subscribe(() => {
this.screenSize$.next({
isExtraSmall: window.innerWidth <= SCREEN_BREAKPOINTS.EXTRA_SMALL,
isSmall: window.innerWidth <= SCREEN_BREAKPOINTS.SMALL
});
});
}
get screenSize() {
return this.screenSize$.asObservable();
}
}This way, all components can dance to the same tune! 🕺
| <ng-container *ngIf="!isSmallScreen && value > -1000000000 && value < 1000000000; else humanize"> | ||
| {{ { value: value, currencyCode: currency, skipSymbol: true } | exactCurrency }} | ||
| </ng-container> | ||
| <ng-template #humanize> | ||
| {{ value | humanizeCurrency : currency : true }} | ||
| </ng-template> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
When numbers are big, style becomes small - that's the rule of currency display!
The conditional currency display is like my signature move - powerful but needs precision! A few style points:
- The billion-value threshold (-1B to 1B) needs validation:
- *ngIf="!isSmallScreen && value > -1000000000 && value < 1000000000; else humanize"
+ *ngIf="!isSmallScreen && value > -1e9 && value < 1e9; else humanize"- Consider extracting these magic numbers to constants, like how I never reveal all my power at once!
Committable suggestion skipped: line range outside the PR's diff.
| <ng-container | ||
| *ngIf=" | ||
| !isSmallScreen && | ||
| cardDetail.stats.totalDraftValue > -1000000000 && | ||
| cardDetail.stats.totalDraftValue < 1000000000; | ||
| else humanizeDraftAmount | ||
| " | ||
| > | ||
| {{ | ||
| { value: cardDetail.stats.totalDraftValue, currencyCode: homeCurrency, skipSymbol: true } | exactCurrency | ||
| }} | ||
| </ng-container> | ||
| <ng-template #humanizeDraftAmount> | ||
| {{ cardDetail.stats.totalDraftValue | humanizeCurrency : homeCurrency : true }} | ||
| </ng-template> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mind it! Don't repeat yourself, even if you're as stylish as me!
Like how I never repeat the same punch dialogue twice, we shouldn't repeat the same template logic! The currency display condition is duplicated for both draft and total amounts.
Let's create a reusable template, macha:
+ <ng-template #currencyDisplay let-value="value">
+ <ng-container *ngIf="!isSmallScreen && value > -1e9 && value < 1e9; else humanizeAmount">
+ {{ { value: value, currencyCode: homeCurrency, skipSymbol: true } | exactCurrency }}
+ </ng-container>
+ <ng-template #humanizeAmount>
+ {{ value | humanizeCurrency : homeCurrency : true }}
+ </ng-template>
+ </ng-template>
<!-- Use it like this -->
- <ng-container *ngIf="!isSmallScreen && cardDetail.stats.totalDraftValue > -1000000000...">
- {{ { value: cardDetail.stats.totalDraftValue, ... } | exactCurrency }}
- </ng-container>
- <ng-template #humanizeDraftAmount>
- {{ cardDetail.stats.totalDraftValue | humanizeCurrency : homeCurrency : true }}
- </ng-template>
+ <ng-container *ngTemplateOutlet="currencyDisplay; context: { value: cardDetail.stats.totalDraftValue }">
+ </ng-container>Also applies to: 60-74
| @@ -3,6 +3,7 @@ import { IonicModule } from '@ionic/angular'; | |||
| import { getElementBySelector, getTextContent } from 'src/app/core/dom-helpers'; | |||
| import { FyCurrencyPipe } from 'src/app/shared/pipes/fy-currency.pipe'; | |||
| import { HumanizeCurrencyPipe } from 'src/app/shared/pipes/humanize-currency.pipe'; | |||
| import { ExactCurrencyPipe } from 'src/app/shared/pipes/exact-currency.pipe'; | |||
There was a problem hiding this comment.
Testing is like my stunts - it needs to cover everything!
Mind it! We've added ExactCurrencyPipe but where are the tests for our new currency display logic? Let's add some power-packed test cases:
it('should use exact currency for values within range on large screens', () => {
component.isSmallScreen = false;
component.value = 999999999;
fixture.detectChanges();
const amountElement = getElementBySelector(fixture, '.stat-badge--amount');
expect(amountElement.textContent).toContain(exactCurrencyPipe.transform({
value: component.value,
currencyCode: 'USD',
skipSymbol: true
}));
});
it('should use humanized currency for values outside range', () => {
component.value = 1000000001;
fixture.detectChanges();
const amountElement = getElementBySelector(fixture, '.stat-badge--amount');
expect(amountElement.textContent).toContain(humanizeCurrencyPipe.transform(
component.value,
'USD',
true
));
});Also applies to: 18-18
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
src/app/fyle/dashboard/stat-badge/stat-badge.component.html(3 hunks)src/app/fyle/dashboard/stat-badge/stat-badge.component.scss(1 hunks)src/app/fyle/dashboard/stat-badge/stat-badge.component.spec.ts(2 hunks)src/app/fyle/dashboard/stat-badge/stat-badge.component.ts(1 hunks)src/app/fyle/dashboard/stats/stats.component.scss(2 hunks)src/app/shared/components/spent-cards/card-detail/card-detail.component.html(2 hunks)src/app/shared/components/spent-cards/card-detail/card-detail.component.scss(1 hunks)src/app/shared/components/spent-cards/card-detail/card-detail.component.spec.ts(2 hunks)src/app/shared/components/spent-cards/card-detail/card-detail.component.ts(1 hunks)
🔇 Additional comments (9)
src/app/fyle/dashboard/stat-badge/stat-badge.component.scss (2)
79-80: Mind-blowing style update for amount display, thalaiva! 🕶️
The new font size and color properties create a perfect balance between readability and style. Just like how I handle action scenes with perfect timing, these styles will handle the currency display with perfect precision!
84-84: Superstar level consistency in typography, macha! 💫
Reducing the name font-size to 12px maintains a harmonious rhythm with other text elements. Like how I maintain my style across all movies, this maintains style across all screen sizes!
src/app/fyle/dashboard/stats/stats.component.scss (1)
23-23: Mind-blowing color upgrade, boss!
Switching to $black-light is a superstar move! Better contrast means better readability for our users. Simply... SUPERB! 🌟
src/app/shared/components/spent-cards/card-detail/card-detail.component.scss (3)
44-44: Mind it! The alignment is perfect, just like my punch dialogues!
The center alignment brings perfect balance to the force... I mean, to the stats display. When you align things in the center, they become unstoppable!
51-56: Kabali da! These styles are as solid as my signature walk!
The font styling is perfectly balanced - not too bold, not too light. Just like how I time my stunts, these numbers will look spectacular on screen!
60-60: 🧹 Nitpick (assertive)
Listen carefully! The font size reduction needs a double take!
While making text smaller is like my famous "mini" dialogues - short but powerful, we need to ensure it remains readable on bigger screens, macha!
Consider adding media queries for larger screens:
@media (min-width: 768px) {
.stats {
&--ccc-classified-amount {
font-size: 16px;
}
&--ccc-stats-title {
font-size: 16px;
}
}
}Also applies to: 66-66
✅ Verification successful
Mind it! These font size changes are perfectly styled, partner!
Listen here, the reduced font sizes are not just a style statement, they're a calculated move! The component maintains excellent readability because:
- The text has proper line heights (28px for amounts, 20px for titles)
- The font weights are well-balanced (500 for amounts)
- The color contrast is strong (using $black and $black-2)
- The component has generous padding (64px 32px) and proper spacing
The component is already responsive with:
- Fluid width (100% - 24px)
- Centered positioning
- Proper spacing between elements
No need for larger font sizes on bigger screens, these sizes are like my punch dialogues - compact but powerful! The design maintains hierarchy and readability across all screen sizes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these components have any media queries for larger screens
rg -g '*.scss' -A 5 '@media.*min-width'
Length of output: 41
Script:
#!/bin/bash
# Let's check the current file for any existing media queries and responsive design patterns
cat src/app/shared/components/spent-cards/card-detail/card-detail.component.scss
# Also check for any responsive design patterns in related files
fd -e scss -p 'spent-cards' --exec cat {}
Length of output: 3189
src/app/fyle/dashboard/stat-badge/stat-badge.component.ts (1)
Line range hint 31-39: Mind it! The void return type addition is perfect!
The explicit void return type makes the code more kabali... I mean, more robust! This is how we do it in style!
src/app/fyle/dashboard/stat-badge/stat-badge.component.html (1)
5-5: The grid column resizing is perfect, like my signature walk!
The adjustment from 4-8 to 3-9 split gives better space utilization. Superstar approves! 🌟
Also applies to: 26-26
src/app/shared/components/spent-cards/card-detail/card-detail.component.html (1)
28-42: 🧹 Nitpick (assertive)
Mind it! Let's handle those billion-dollar edge cases, partner!
The condition for switching between exact and humanized currency looks solid, but what happens when the value hits exactly -1B or 1B? In the world of high finance, every rupee counts!
Consider this style, macha:
- !isSmallScreen &&
- cardDetail.stats.totalDraftValue > -1000000000 &&
- cardDetail.stats.totalDraftValue < 1000000000;
+ !isSmallScreen &&
+ Math.abs(cardDetail.stats.totalDraftValue) <= 1000000000;✅ Verification successful
Let me check how these values are processed, partner! Need to see if there's any special handling in the component or the currency pipes. Mind it!
Mind it! The billion-dollar logic is perfectly safe, no need to flip the style!
Looking at the code implementation, I can confidently say the current approach is rock solid! Here's why, partner:
- The HumanizeCurrencyPipe automatically handles values beyond billions using SI units ('B', 't', 'q'...), making it perfect for the fallback case
- The ExactCurrencyPipe handles the sign separately and uses absolute values, ensuring precise formatting for edge cases
- The current condition
> -1000000000 && < 1000000000explicitly excludes the billion-dollar boundaries, making them fall into the humanized format - which is exactly what we want!
Your suggested refactor using Math.abs() <= 1000000000 would actually include the billion-dollar values in the exact format, which isn't as readable for such large numbers. The current style is more powerful than a punch dialogue, mind it!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any transactions near the billion-dollar threshold
rg -l "totalDraftValue|totalAmountValue" | xargs rg "1000000000|1,000,000,000"
Length of output: 648
Script:
#!/bin/bash
# Let's check the component's TypeScript file for any value handling logic
ast-grep --pattern 'class CardDetailComponent {
$$$
}' src/app/shared/components/spent-cards/card-detail/card-detail.component.ts
# Also check for any currency pipe implementations
fd "exact-currency|humanize-currency" --exec cat {}
Length of output: 13654
| @@ -87,14 +87,12 @@ $ccc-bank-name: #c4cae8; | |||
| } | |||
|
|
|||
| &--report-stats-row { | |||
| padding-bottom: 18px; | |||
| padding-top: 18px; | |||
| padding: 24px 0 20px 0; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Style simplification that packs a punch!
Mind it! The padding consolidation is like my signature move - simple yet powerful! Both rows now use the same clean shorthand syntax. But let me tell you something interesting - we could make this even more powerful!
Consider extracting this common padding to a shared class or variable, like how I share my style with millions:
.stats {
+ $stats-row-padding: 24px 0 20px 0;
&--report-stats-row {
- padding: 24px 0 20px 0;
+ padding: $stats-row-padding;
--ion-grid-column-padding: 0;
}
&--expenses-stats-row {
- padding: 24px 0 20px 0;
+ padding: $stats-row-padding;
--ion-grid-column-padding: 0;
}
}Also applies to: 95-95
| // To track if the screen is small (370px or below) | ||
| isSmallScreen = window.innerWidth <= 370; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mind blowing! But let's make this magic number a constant, partner!
When I see magic numbers like 370, I say to them - "Don't act smart!" Let's move this to a constant at the top of the file, macha!
+ private readonly SMALL_SCREEN_THRESHOLD = 370;
- isSmallScreen = window.innerWidth <= 370;
+ isSmallScreen = window.innerWidth <= this.SMALL_SCREEN_THRESHOLD;📝 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.
| // To track if the screen is small (370px or below) | |
| isSmallScreen = window.innerWidth <= 370; | |
| private readonly SMALL_SCREEN_THRESHOLD = 370; | |
| // To track if the screen is small (370px or below) | |
| isSmallScreen = window.innerWidth <= this.SMALL_SCREEN_THRESHOLD; |
| // To track if the screen is small (320px or below) | ||
| isSmallScreen = window.innerWidth <= 320; |
There was a problem hiding this comment.
Ey! What's this? Different screen sizes dancing different steps!
Listen here, partner! I'm seeing 320px here but 370px in StatBadgeComponent. This inconsistency is like serving different curries in the same dish! Let's standardize this across components!
Consider:
- Create a shared constant for small screen threshold
- Use the same value across components
- Document why we chose this specific value
+// Move to a shared constants file
+export const SMALL_SCREEN_THRESHOLD = 370; // or 320, but pick one!
+
@Component({
selector: 'app-card-detail',
templateUrl: './card-detail.component.html',
styleUrls: ['./card-detail.component.scss'],
})
export class CardDetailComponent {
- isSmallScreen = window.innerWidth <= 320;
+ isSmallScreen = window.innerWidth <= SMALL_SCREEN_THRESHOLD;Committable suggestion skipped: line range outside the PR's diff.
| <ng-container *ngIf="!isSmallScreen && value > -1000000000 && value < 1000000000; else humanize"> | ||
| {{ { value: value, currencyCode: currency, skipSymbol: true } | exactCurrency }} | ||
| </ng-container> | ||
| <ng-template #humanize> | ||
| {{ value | humanizeCurrency : currency : true }} | ||
| </ng-template> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Style ah? Let's make this template as clean as my sunglasses!
That condition is longer than my punch dialogues! Let's move it to the component, machan!
- Move the magic numbers to constants:
+ private readonly MAX_EXACT_CURRENCY_VALUE = 1000000000;
+ private readonly MIN_EXACT_CURRENCY_VALUE = -1000000000;- Add a method to handle the condition:
+ shouldShowExactCurrency(value: number): boolean {
+ return !this.isSmallScreen &&
+ value > this.MIN_EXACT_CURRENCY_VALUE &&
+ value < this.MAX_EXACT_CURRENCY_VALUE;
+ }- Simplify the template:
- <ng-container *ngIf="!isSmallScreen && value > -1000000000 && value < 1000000000; else humanize">
+ <ng-container *ngIf="shouldShowExactCurrency(value); else humanize">Mind it! This makes the code more maintainable and readable!
Committable suggestion skipped: line range outside the PR's diff.
| <ng-container | ||
| *ngIf=" | ||
| !isSmallScreen && | ||
| cardDetail.stats.totalAmountValue > -1000000000 && | ||
| cardDetail.stats.totalAmountValue < 1000000000; | ||
| else humanizeTotalAmount | ||
| " | ||
| > | ||
| {{ | ||
| { value: cardDetail.stats.totalAmountValue, currencyCode: homeCurrency, skipSymbol: true } | exactCurrency | ||
| }} | ||
| </ng-container> | ||
| <ng-template #humanizeTotalAmount> | ||
| {{ cardDetail.stats.totalAmountValue | humanizeCurrency : homeCurrency : true }} | ||
| </ng-template> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Kabali style code duplication detected!
Like how I don't repeat my punch dialogues, we shouldn't repeat our code logic! The same condition structure appears twice.
Let's create a reusable template, thalaiva:
+ <ng-template #exactAmount let-value="value">
+ <ng-container
+ *ngIf="
+ !isSmallScreen &&
+ Math.abs(value) <= 1000000000;
+ else humanizeAmount
+ "
+ >
+ {{
+ { value: value, currencyCode: homeCurrency, skipSymbol: true } | exactCurrency
+ }}
+ </ng-container>
+ </ng-template>
+
+ <ng-template #humanizeAmount let-value="value">
+ {{ value | humanizeCurrency : homeCurrency : true }}
+ </ng-template>
- <ng-container *ngIf="...">
- {{ ... }}
- </ng-container>
+ <ng-container
+ *ngTemplateOutlet="exactAmount; context: { value: cardDetail.stats.totalAmountValue }"
+ >
+ </ng-container>Committable suggestion skipped: line range outside the PR's diff.
| @@ -3,6 +3,7 @@ import { IonicModule } from '@ionic/angular'; | |||
| import { getElementBySelector, getTextContent } from 'src/app/core/dom-helpers'; | |||
| import { FyCurrencyPipe } from 'src/app/shared/pipes/fy-currency.pipe'; | |||
| import { HumanizeCurrencyPipe } from 'src/app/shared/pipes/humanize-currency.pipe'; | |||
| import { ExactCurrencyPipe } from 'src/app/shared/pipes/exact-currency.pipe'; | |||
There was a problem hiding this comment.
Mind it! Test coverage needs more power!
You've added ExactCurrencyPipe to the declarations, but where are the test cases for it? Like my movies need action scenes, your tests need coverage!
Add test cases to verify the ExactCurrencyPipe behavior:
it('should use ExactCurrencyPipe for appropriate values', () => {
component.value = 999999999;
component.isSmallScreen = false;
fixture.detectChanges();
const amountElement = getElementBySelector(fixture, '.stat-badge--amount');
expect(amountElement.textContent).toContain('999,999,999.00');
});Also applies to: 18-18
| @@ -6,6 +6,7 @@ import { IonicModule } from '@ionic/angular'; | |||
| import { TrackingService } from 'src/app/core/services/tracking.service'; | |||
| import { FyCurrencyPipe } from 'src/app/shared/pipes/fy-currency.pipe'; | |||
| import { HumanizeCurrencyPipe } from 'src/app/shared/pipes/humanize-currency.pipe'; | |||
| import { ExactCurrencyPipe } from 'src/app/shared/pipes/exact-currency.pipe'; | |||
There was a problem hiding this comment.
Style ah? Where are the test cases for our new currency superstar?
Like how I test my stunts before performing them, we need to test our new currency display logic! The ExactCurrencyPipe is imported but not properly tested.
Add these test cases, partner:
describe('currency display', () => {
it('should use ExactCurrencyPipe for non-small screens and values within range', () => {
component.isSmallScreen = false;
component.cardDetail.stats.totalAmountValue = 999999999;
fixture.detectChanges();
const amountElement = fixture.debugElement.query(By.css('.stats--ccc-classified-amount'));
expect(amountElement.nativeElement.textContent).toContain('999,999,999.00');
});
it('should fallback to HumanizeCurrencyPipe for values outside range', () => {
component.isSmallScreen = false;
component.cardDetail.stats.totalAmountValue = 1000000001;
fixture.detectChanges();
const amountElement = fixture.debugElement.query(By.css('.stats--ccc-classified-amount'));
expect(amountElement.nativeElement.textContent).toContain('1B');
});
});Also applies to: 45-51
|
Important
This is the feature branch.
This PR contains all the approved child PRs ✅
Clickup
clickup link
Summary by CodeRabbit
New Features
Bug Fixes
Style
Tests
ExactCurrencyPipeto the test suites for the Stat Badge and Card Detail components to enhance testing capabilities.