feat: Milestone 2: Reports view display exact amount #3333
Conversation
WalkthroughThe changes in this pull request focus on enhancing the layout and presentation of the report details in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
SahilK-027
changed the title
![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
🧹 Outside diff range comments (1)
src/app/shared/components/reports-card/reports-card.component.html (1)
Line range hint
49-60: The menu needs some punch, like my signature style!While the implementation is clean, let's make it accessible to keyboard warriors too!
Add keyboard support:
<div matRipple (click)="actionOpened = false" class="reports-card-menu--cancel" + tabindex="0" + (keydown.enter)="actionOpened = false" + role="button" + aria-label="Close menu" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (7)
src/app/fyle/my-reports/my-reports.page.html(1 hunks)src/app/fyle/my-view-report/add-expenses-to-report/add-expenses-to-report.component.html(1 hunks)src/app/fyle/my-view-report/add-expenses-to-report/add-expenses-to-report.component.scss(1 hunks)src/app/fyle/my-view-report/add-expenses-to-report/add-expenses-to-report.component.spec.ts(3 hunks)src/app/shared/components/reports-card/reports-card.component.html(1 hunks)src/app/shared/components/reports-card/reports-card.component.scss(4 hunks)src/app/shared/components/reports-card/reports-card.component.spec.ts(2 hunks)
🔇 Additional comments (8)
src/app/shared/components/reports-card/reports-card.component.scss (3)
4-5: Mind blowing! The card spacing is perfectly balanced, like my movie stunts!
The reduction in padding to 12px and addition of 6px bottom margin creates a more compact and rhythmic layout. This is the way!
97-97: Even the smallest details matter, like my iconic walk!
The no-transactions message styling with reduced font size and $dark-grey color maintains perfect visual hierarchy. That's the mark of a true style master!
Also applies to: 99-99
30-34: Superstar approves this flex container structure!
The new container with flex display and space-between creates perfect balance, like the equilibrium in my action sequences! But remember, with great power comes great responsibility - ensure the width: 100% doesn't cause overflow on smaller screens.
✅ Verification successful
Mind-blowing flex container, but let me show you something, boss!
Looking at the styles, I can see the container is well-protected like my movies! The parent elements have proper width constraints with max-width: 90% and overflow: hidden properties. The flex container's width: 100% works within these boundaries, making it as reliable as my signature moves!
The child elements are also perfectly balanced:
- One takes 30% width
- Another takes 70% width
- Text overflow is handled with ellipsis
This is a blockbuster implementation that won't cause any overflow issues on smaller screens. Style!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential overflow issues in parent containers
rg -l "reports-card" | xargs rg "width|overflow|max-width"
Length of output: 999
src/app/fyle/my-view-report/add-expenses-to-report/add-expenses-to-report.component.scss (1)
10-10: 🧹 Nitpick (assertive)
Mind it! The font size reduction needs attention, macha!
The reduction from 20px to 14px is quite dramatic! While the visual hierarchy with the new total-amount class looks stylish, we need to ensure the title remains prominent enough for users.
Let me check other components' font sizes, thalaiva style:
Consider this style, like a punch dialogue:
&--title {
- font-size: 14px;
+ font-size: 16px;
margin-bottom: 4px;
}Also applies to: 15-21
✅ Verification successful
Based on the font size analysis across the codebase, I can now provide a definitive response:
Style, macha! The font size change is perfectly fine!
The reduction from 20px to 14px aligns perfectly with our app's style patterns. Looking at the codebase:
- Title elements commonly use 14px (appears >100 times)
- 14px is our standard text size across components
- Even important elements like headers and cards use 14-16px range
- 20px is rarely used (only ~15 occurrences) and mostly for special cases
The new size brings better consistency with our design system, thalaiva! No need to revert this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check font sizes across components for consistency
rg -g "*.scss" "font-size:" | grep -v "node_modules"
Length of output: 65617
src/app/fyle/my-reports/my-reports.page.html (1)
75-75: Mind it! The new loader looks promising, but let's verify its behavior!
The addition of [isNewLoaderEnabled]="true" shows style, but like my signature moves, we need to ensure it performs flawlessly across different loading scenarios.
Let me help you verify this change, thalaiva:
✅ Verification successful
Mind it! The new loader is working like a well-oiled machine, partner!
The verification shows the loader component is fully equipped to handle this new style, with proper:
- Input property setup in the component
- Conditional styling with ngClass
- Responsive column sizing adjustments
- Proper skeleton text animations
Just like how I flip my cigarette with style, this loader flips between old and new looks with perfect precision!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other instances of the new loader and its implementation
# Look for other instances of isNewLoaderEnabled
rg "isNewLoaderEnabled" -A 3
# Check the loader component implementation
ast-grep --pattern 'class $_ {
$$$
@Input() isNewLoaderEnabled
$$$
}'
Length of output: 3695
src/app/fyle/my-view-report/add-expenses-to-report/add-expenses-to-report.component.spec.ts (1)
11-11: Kabali style! Adding ExactCurrencyPipe for precise currency formatting!
The addition of ExactCurrencyPipe shows your commitment to precision, just like my perfectly timed punches! The pipe declaration in TestBed is spot on.
Also applies to: 32-32
src/app/shared/components/reports-card/reports-card.component.html (2)
1-5: Mind it! The date section looks perfect, just like my timing in action scenes!
The conditional rendering with *ngIf and date formatting is spot on, partner!
31-37: Listen up! The amount formatting needs a double-check, like my famous double-action scenes!
Let's verify this new exactCurrency pipe implementation across different currency scenarios.
✅ Verification successful
Let me explain why I need this information, partner! The previous search showed us where the pipe is used, but we need to verify:
- The actual implementation of ExactCurrencyPipe
- Whether it has proper test coverage
- How it handles different currency scenarios
The new search will help us find these details with style! 🕶️
Mind it! We found the implementation, but like in my movies where I verify every move before the action, we need to check:
- The test cases to ensure all currency scenarios are covered
- The FyCurrencyPipe that our ExactCurrencyPipe depends on
This will give us the complete picture, just like a perfect climax scene! 🎬
Mind it! The currency formatting is rock-solid, just like my punch dialogues!
The ExactCurrencyPipe is a blockbuster implementation, partner! Let me tell you why:
- Handles positive and negative values with style ✨
- Takes care of currency symbols and fractions like a boss 💪
- Test coverage is comprehensive, covering all scenarios from zero to millions
- Even handles edge cases like missing parameters and decimal precision
The usage in reports-card.component.html is perfectly aligned with the implementation:
{ value: report.amount || 0, currencyCode: report.currency, skipSymbol: true }When I say something works, it works! 🕶️
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all exactCurrency pipe usages and their test coverage
echo "Checking exactCurrency pipe usage:"
rg "exactCurrency" -t html -A 2
echo "Checking test coverage:"
rg "ExactCurrencyPipe" -t typescript
Length of output: 1306
Script:
#!/bin/bash
# Let's search for the ExactCurrencyPipe implementation and its tests
echo "Searching for ExactCurrencyPipe implementation:"
fd "exact-currency" --type f
rg "export.*ExactCurrencyPipe" -A 10
echo -e "\nSearching for ExactCurrencyPipe tests:"
fd "exact-currency.*.spec" --type f
Length of output: 1499
Script:
#!/bin/bash
# Let's check the test coverage with style!
echo "Checking the test cases, mind it!"
cat src/app/shared/pipes/exact-currency.pipe.spec.ts
echo -e "\nLet's also check the FyCurrencyPipe it depends on:"
rg "export.*FyCurrencyPipe" -A 5
Length of output: 6165
| margin-bottom: 16px; | ||
| } | ||
|
|
||
| &--date { | ||
| color: $grey-light; | ||
| color: $black-light; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Listen carefully! The date section needs more breathing space!
The color change to $black-light improves readability, but the padding 24px 14px 16px 14px seems inconsistent. When I say style, I mean symmetrical style!
- padding: 24px 14px 16px 14px;
+ padding: 16px 14px;Also applies to: 19-19
| display: flex; | ||
| gap: 8px; | ||
| margin-bottom: 6px; | ||
| width: 80%; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Kabali says: This layout restructuring is a masterpiece!
The combination of:
- Main info block with 80% width and 8px gap
- Purpose-num-amount block with column layout
- Amount block with top margin
Creates a perfect visual hierarchy, like the climax scene in my movies! But the main info width of 80% might need adjustment based on content.
Consider using CSS Grid for more flexible layouts:
- &--main-info {
- display: flex;
- gap: 8px;
- margin-bottom: 6px;
- width: 80%;
- }
+ &--main-info {
+ display: grid;
+ grid-template-columns: minmax(0, 1fr) auto;
+ gap: 8px;
+ margin-bottom: 6px;
+ }Also applies to: 61-64, 75-77
| font-size: 14px; | ||
| font-weight: 500; | ||
| max-width: 60%; | ||
| max-width: 90%; | ||
| overflow: hidden; | ||
| text-overflow: ellipsis; | ||
| white-space: nowrap; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
The typography changes are as sharp as my signature sunglasses!
The text overflow handling is perfect:
- Font size reduction to 14px
- Max-width increase to 90%
- Proper overflow handling with ellipsis
But mind you, make sure the truncated text has a tooltip for accessibility!
Consider adding a title attribute for truncated text:
&--purpose {
&[title] {
cursor: help;
}
}| declarations: [ | ||
| ReportsCardComponent, | ||
| EllipsisPipe, | ||
| HumanizeCurrencyPipe, | ||
| ExactCurrencyPipe, | ||
| ReportState, | ||
| SnakeCaseToSpaceCase, | ||
| ], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Like my signature move, we need a powerful finish to these tests!
The ExactCurrencyPipe addition is good, but where are the test cases for the new currency formatting? A superstar test suite needs to verify all scenarios!
Add these test cases, and make them as powerful as Thalaiva:
it('should format currency correctly with ExactCurrencyPipe', () => {
component.report.amount = 1234567.89;
fixture.detectChanges();
expect(getTextContent(getElementBySelector(fixture, '.reports-card--amount')))
.toEqual('1,234,567.89');
});
it('should handle zero amount correctly', () => {
component.report.amount = 0;
fixture.detectChanges();
expect(getTextContent(getElementBySelector(fixture, '.reports-card--amount')))
.toEqual('0.00');
});| {{ selectedElements?.length }} {{ selectedElements?.length > 1 ? 'Expenses' : 'Expense' }} | ||
| </div> | ||
| <div class="add-expenses-to-report--total-amount"> | ||
| {{ { value: selectedTotalAmount || 0, currencyCode: homeCurrency } | exactCurrency }} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Kabali style approval! But let's make it accessible for everyone!
The separation of expense count and amount is as clean as my style! The exactCurrency pipe brings precision like my action sequences.
Add ARIA labels for better accessibility, like how I make sure everyone in the audience enjoys the show:
<div class="add-expenses-to-report--title">
- {{ selectedElements?.length }} {{ selectedElements?.length > 1 ? 'Expenses' : 'Expense' }}
+ <span aria-label="Selected expenses count">
+ {{ selectedElements?.length }} {{ selectedElements?.length > 1 ? 'Expenses' : 'Expense' }}
+ </span>
</div>
<div class="add-expenses-to-report--total-amount">
- {{ { value: selectedTotalAmount || 0, currencyCode: homeCurrency } | exactCurrency }}
+ <span aria-label="Total amount">
+ {{ { value: selectedTotalAmount || 0, currencyCode: homeCurrency } | exactCurrency }}
+ </span>
</div>📝 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.
| {{ selectedElements?.length }} {{ selectedElements?.length > 1 ? 'Expenses' : 'Expense' }} | |
| </div> | |
| <div class="add-expenses-to-report--total-amount"> | |
| {{ { value: selectedTotalAmount || 0, currencyCode: homeCurrency } | exactCurrency }} | |
| <span aria-label="Selected expenses count"> | |
| {{ selectedElements?.length }} {{ selectedElements?.length > 1 ? 'Expenses' : 'Expense' }} | |
| </span> | |
| </div> | |
| <div class="add-expenses-to-report--total-amount"> | |
| <span aria-label="Total amount"> | |
| {{ { value: selectedTotalAmount || 0, currencyCode: homeCurrency } | exactCurrency }} | |
| </span> |
| it('should show number of expenses', () => { | ||
| component.selectedElements = [expense1, expense2]; | ||
| fixture.detectChanges(); | ||
|
|
||
| expect(getTextContent(getElementBySelector(fixture, '.add-expenses-to-report--title'))).toEqual( | ||
| '2 Expenses - $500.00' | ||
| ); | ||
| expect(getTextContent(getElementBySelector(fixture, '.add-expenses-to-report--title'))).toEqual('2 Expenses'); | ||
| }); | ||
|
|
||
| it('should show total amount', () => { | ||
| component.selectedElements = [expense1, expense2]; | ||
| fixture.detectChanges(); | ||
|
|
||
| expect(getTextContent(getElementBySelector(fixture, '.add-expenses-to-report--total-amount'))).toEqual('$500.00'); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Single responsibility, double the impact! Like my double-role movies!
Breaking down the test into two separate cases - one for expense count and another for total amount - is a superb move! This makes the tests more focused and easier to maintain.
Consider adding these test cases to verify edge cases:
it('should show singular expense text when only one expense is selected', () => {
component.selectedElements = [expense1];
fixture.detectChanges();
expect(getTextContent(getElementBySelector(fixture, '.add-expenses-to-report--title'))).toEqual('1 Expense');
});
it('should show zero amount when no expenses are selected', () => {
component.selectedElements = [];
fixture.detectChanges();
expect(getTextContent(getElementBySelector(fixture, '.add-expenses-to-report--total-amount'))).toEqual('$0.00');
});| <!-- Report cards --> | ||
| <div | ||
| class="reports-card" | ||
| [ngClass]="{ 'reports-card__with-border': !showDate }" | ||
| (click)="onGoToReport()" | ||
| *ngIf="!actionOpened" | ||
| matRipple | ||
| > | ||
| <div class="reports-card--divider" *ngIf="!showDate"></div> | ||
| <div class="reports-card--main-info"> | ||
| <div class="reports-card--purpose-amount-block"> | ||
| <div class="reports-card--purpose"> | ||
| <span>{{ report.purpose | ellipsis : 40 }}</span> | ||
| </div> | ||
| <div class="reports-card--amount-block ion-text-nowrap"> | ||
| <span class="reports-card--currency">{{ reportCurrencySymbol }}</span> | ||
| <span class="reports-card--amount"> | ||
| {{ report.amount || 0 | humanizeCurrency : report.currency : true }} | ||
| </span> | ||
| <div class="reports-card--container"> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Kabali da! The card structure is solid as a rock, but let's make it even better!
The ripple effect and conditional styling show style, but like how I care for my fans, we should care for all users!
Add ARIA attributes to improve accessibility:
<div
class="reports-card"
[ngClass]="{ 'reports-card__with-border': !showDate }"
(click)="onGoToReport()"
*ngIf="!actionOpened"
matRipple
+ role="button"
+ [attr.aria-label]="'View report: ' + report.purpose"
>Committable suggestion skipped: line range outside the PR's diff.
| <div class="reports-card--purpose-num-amount-block"> | ||
| <!-- Report name --> | ||
| <div class="reports-card--purpose"> | ||
| <span>{{ report.purpose | ellipsis : 40 }}</span> | ||
| </div> | ||
|
|
||
| <!-- Number of expenses --> | ||
| <div class="reports-card--no-transactions"> | ||
| <span>{{ report.num_expenses }} </span> | ||
| <span>{{ report.num_expenses === 1 ? 'Expense' : 'Expenses' }}</span> | ||
| </div> | ||
|
|
||
| <!-- Total amount --> | ||
| <div class="reports-card--amount-block ion-text-nowrap"> | ||
| <span class="reports-card--currency">{{ reportCurrencySymbol }}</span> | ||
| <span class="reports-card--amount"> | ||
| {{ { value: report.amount || 0, currencyCode: report.currency, skipSymbol: true } | exactCurrency }} | ||
| </span> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| <div class="reports-card--secondary-info"> | ||
| <div class="reports-card--secondary-info-block"> | ||
| <div class="reports-card--no-transactions"> | ||
| <span>{{ report.num_expenses }} </span> | ||
| <span>{{ report.num_expenses === 1 ? 'Expense' : 'Expenses' }}</span> | ||
| </div> | ||
| <div class="state-pill state-{{ report.state | reportState }}"> | ||
| {{ report.state | reportState : simplifyReportsEnabled | snakeCaseToSpaceCase }} | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Style ah? Structure ah? Why not both! Let's make this code as powerful as my punch dialogues!
The nested structure could be simplified for better maintainability.
Consider breaking down the purpose-num-amount block into a separate component:
-<div class="reports-card--purpose-num-amount-block">
- <!-- Current content -->
-</div>
+<app-report-details
+ [purpose]="report.purpose"
+ [numExpenses]="report.num_expenses"
+ [amount]="report.amount"
+ [currency]="report.currency"
+ [currencySymbol]="reportCurrencySymbol">
+</app-report-details>Committable suggestion skipped: line range outside the PR's diff.
|
Important
This PR contains all the approved child PRs ✅
Clickup
clickup link
Summary by CodeRabbit
New Features
Style
Tests
ExactCurrencyPipein the test suite for improved testing coverage.