Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Milestone 2: Reports view display exact amount #3333

Merged
merged 3 commits into from
Dec 19, 2024
Merged

feat: Milestone 2: Reports view display exact amount #3333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f4a4cd9
feat: Update view report (reports--card) to display the exact amount …
SahilK-027 Dec 11, 2024
044025c
feat: Add expenses to report (display exact amount) (#3331)
SahilK-027 Dec 11, 2024
2884198
feat: Report card section display exact amount (#3328)
SahilK-027 Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/app/fyle/my-reports/my-reports.page.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@
</ng-container>

<ng-template #loading>
<app-fy-loading-screen class="my-reports--shimmers"></app-fy-loading-screen>
<app-fy-loading-screen [isNewLoaderEnabled]="true" class="my-reports--shimmers"></app-fy-loading-screen>
</ng-template>
<div *ngIf="(count$ | async) === 0" class="my-reports--zero-state">
<app-fy-zero-state
Expand Down
6 changes: 4 additions & 2 deletions src/app/fyle/my-view-report/add-expenses-to-report/add-expenses-to-report.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
<div *ngIf="selectedElements?.length === 0" class="report-list--title text-center">Add Expenses</div>
<div *ngIf="selectedElements?.length > 0" class="add-expenses-to-report--title-container text-center">
<div class="add-expenses-to-report--title">
{{ selectedElements?.length }} {{ selectedElements?.length > 1 ? 'Expenses' : 'Expense' }} -
{{ selectedTotalAmount || 0 | humanizeCurrency : homeCurrency }}
{{ selectedElements?.length }} {{ selectedElements?.length > 1 ? 'Expenses' : 'Expense' }}
</div>
<div class="add-expenses-to-report--total-amount">
{{ { value: selectedTotalAmount || 0, currencyCode: homeCurrency } | exactCurrency }}
Comment on lines +8 to +11
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
{{ 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>

</div>
</div>
</ion-title>
Expand Down
12 changes: 8 additions & 4 deletions src/app/fyle/my-view-report/add-expenses-to-report/add-expenses-to-report.component.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,18 @@
}

&--title-container {
color: $black;
color: $blue-black;
font-weight: 500;
margin-left: -10%;
}

&--title {
font-size: 20px;
line-height: 26px;
font-size: 14px;
margin-bottom: 4px;
}

&--total-amount {
font-size: 12px;
font-weight: 400;
}

&--close {
Expand Down
16 changes: 11 additions & 5 deletions src/app/fyle/my-view-report/add-expenses-to-report/add-expenses-to-report.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { click, getElementBySelector, getTextContent } from 'src/app/core/dom-he
import { CurrencyService } from 'src/app/core/services/currency.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';
import { AddExpensesToReportComponent } from './add-expenses-to-report.component';
import { expenseData } from 'src/app/core/mock-data/platform/v1/expense.data';

Expand All @@ -28,7 +29,7 @@ describe('AddExpensesToReportComponent', () => {
const routerSpy = jasmine.createSpyObj('Router', ['navigate']);

TestBed.configureTestingModule({
declarations: [AddExpensesToReportComponent, HumanizeCurrencyPipe],
declarations: [AddExpensesToReportComponent, HumanizeCurrencyPipe, ExactCurrencyPipe],
imports: [IonicModule.forRoot()],
providers: [
FyCurrencyPipe,
Expand Down Expand Up @@ -194,13 +195,18 @@ describe('AddExpensesToReportComponent', () => {
expect(getTextContent(getElementBySelector(fixture, '.report-list--title'))).toEqual('Add Expenses');
});

it('should show number of expenses and total amount', () => {
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');
Comment on lines +198 to +209
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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');
});

});

it('should zero state message if no unreported expense exist', () => {
Expand Down
31 changes: 15 additions & 16 deletions src/app/fyle/my-view-report/my-view-report.page.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@
<div class="view-reports--wrapper">
<div class="view-reports--card">
<ion-grid>
<ion-row>
<ion-col size="8">
<ion-row class="view-reports--purpose-state">
<ion-col size="8.5">
<div class="view-reports--purpose-amount-block">
<div class="view-reports--purpose-amount-block__purpose">
{{report.purpose | ellipsis: 15}}
{{report.purpose | ellipsis: 25}}
<ion-icon
*ngIf="(canEdit$|async) && report && (report.state === 'DRAFT' || report.state === 'APPROVER_INQUIRY' || report.state === 'APPROVER_PENDING')"
[src]="'../../../../assets/svg/edit.svg'"
Expand All @@ -44,19 +44,7 @@
</div>
</div>
</ion-col>
<ion-col size="4">
<div class="view-reports--amount ion-text-nowrap">
<span class="view-reports--purpose-amount-block__currency">{{ reportCurrencySymbol }}</span>
<span class="view-reports--purpose-amount-block__amount">
{{ report.amount || 0 | humanizeCurrency: report.currency:true }}
</span>
</div>
</ion-col>
</ion-row>

<ion-row>
<ion-col size="5"> </ion-col>
<ion-col size="7">
<ion-col size="3.5">
<ng-container *ngIf="(simplifyReportsSettings$ | async) as simplifyReportsSettings">
<div class="view-reports--report-state-pill state-pill state-{{report.state | reportState}}">
{{report.state | reportState : simplifyReportsSettings.enabled | snakeCaseToSpaceCase}}
Expand All @@ -65,6 +53,17 @@
</ion-col>
</ion-row>

<ion-row>
<ion-col size="8">
<div class="view-reports--amount ion-text-nowrap">
<span class="view-reports--purpose-amount-block__currency">{{ reportCurrencySymbol }}</span>
<span class="view-reports--purpose-amount-block__amount">
{{ { value: report.amount || 0, currencyCode: report.currency, skipSymbol: true } | exactCurrency }}
</span>
</div>
</ion-col>
</ion-row>

<ion-row class="view-reports--date-info-block">
<ion-col size="8">
<div>
Expand Down
14 changes: 10 additions & 4 deletions src/app/fyle/my-view-report/my-view-report.page.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,13 @@

&--amount {
color: $white;
float: right;
font-size: 18px;
}

&--purpose-state {
display: flex;
justify-content: space-between;
align-items: center;
}

&--purpose-amount-block {
Expand All @@ -109,14 +115,14 @@
color: $pure-white;

&__purpose {
font-size: 16px;
font-size: 14px;
line-height: 18px;
font-weight: 500;
max-width: 85%;
max-width: 100%;
}

&__amount {
font-size: 20px;
font-size: 18px;
font-weight: 500;
}
}
Expand Down
2 changes: 2 additions & 0 deletions src/app/fyle/my-view-report/my-view-report.page.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import { ToastMessageComponent } from 'src/app/shared/components/toast-message/t
import { EllipsisPipe } from 'src/app/shared/pipes/ellipses.pipe';
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';
import { ReportState } from 'src/app/shared/pipes/report-state.pipe';
import { SnakeCaseToSpaceCase } from 'src/app/shared/pipes/snake-case-to-space-case.pipe';
import { NetworkService } from '../../core/services/network.service';
Expand Down Expand Up @@ -124,6 +125,7 @@ describe('MyViewReportPage', () => {
MyViewReportPage,
EllipsisPipe,
HumanizeCurrencyPipe,
ExactCurrencyPipe,
ReportState,
SnakeCaseToSpaceCase,
AsyncPipe,
Expand Down
54 changes: 34 additions & 20 deletions src/app/shared/components/reports-card/reports-card.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,49 @@
<!-- Report Date -->
<div *ngIf="showDate" class="reports-card--date">
{{ report.created_at | date : 'MMM dd, YYYY' }}
</div>

<!-- 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">
Comment on lines +6 to +14
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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--main-info">
<!-- Icon -->
<mat-icon class="reports-card--list-icon" svgIcon="list"></mat-icon>

<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 }}

Comment on lines +19 to +40
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

<!-- Report status -->
<div class="reports-card--secondary-info">
<div class="reports-card--secondary-info-block">
<div class="state-pill state-{{ report.state | reportState }}">
{{ report.state | reportState : simplifyReportsEnabled | snakeCaseToSpaceCase }}
</div>
</div>
</div>
</div>
Expand Down
51 changes: 34 additions & 17 deletions src/app/shared/components/reports-card/reports-card.component.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,22 @@
@import '../../../../theme/colors.scss';

.reports-card {
padding: 16px;
padding: 12px;
margin-bottom: 6px;
background-color: $pure-white;

&__with-border {
padding-top: 0;
}

&__no-top-margin {
margin-top: 0;
}

&--divider {
border-bottom: 1px solid $grey-lighter;
margin-bottom: 16px;
}

&--date {
color: $grey-light;
color: $black-light;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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

font-size: 14px;
padding: 16px 0 12px 16px;
padding: 24px 14px 16px 14px;
background-color: $white;
font-weight: 500;
}
Expand All @@ -31,8 +27,23 @@
margin-right: 20px;
}

&--container {
display: flex;
justify-content: space-between;
width: 100%;
}

&--main-info {
display: flex;
gap: 8px;
margin-bottom: 6px;
width: 80%;
}
Comment on lines +37 to +41
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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


&--list-icon {
width: 16px;
height: 16px;
padding-top: 2px;
}

&--icon-policy-violation {
Expand All @@ -47,9 +58,10 @@
justify-content: space-between;
}

&--purpose-amount-block {
&--purpose-num-amount-block {
display: flex;
justify-content: space-between;
flex-direction: column;
width: 100%;
}

&--currency {
Expand All @@ -60,26 +72,31 @@
margin-right: -2px;
}

&--amount-block {
margin-top: 10px;
}

&--amount {
color: $black;
font-size: 20px;
font-size: 16px;
line-height: 1.3;
margin-right: 6px;
font-weight: 500;
}

&--purpose {
line-height: 23px;
color: $black-2;
font-size: 18px;
font-size: 14px;
font-weight: 500;
max-width: 60%;
max-width: 90%;
overflow: hidden;
text-overflow: ellipsis;
white-space: nowrap;
Comment on lines +88 to +93
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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;
  }
}

}

&--no-transactions {
font-size: 14px;
font-size: 12px;
line-height: 18px;
color: $blue-black;
color: $dark-grey;
}
}

Expand Down
10 changes: 9 additions & 1 deletion src/app/shared/components/reports-card/reports-card.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { MatIconTestingModule } from '@angular/material/icon/testing';
import { MatIconModule } from '@angular/material/icon';
import { EllipsisPipe } from '../../pipes/ellipses.pipe';
import { HumanizeCurrencyPipe } from '../../pipes/humanize-currency.pipe';
import { ExactCurrencyPipe } from '../../pipes/exact-currency.pipe';
import { FyCurrencyPipe } from '../../pipes/fy-currency.pipe';
import { CurrencyPipe } from '@angular/common';
import { ReportState } from '../../pipes/report-state.pipe';
Expand All @@ -18,7 +19,14 @@ describe('ReportsCardComponent', () => {

beforeEach(waitForAsync(() => {
TestBed.configureTestingModule({
declarations: [ReportsCardComponent, EllipsisPipe, HumanizeCurrencyPipe, ReportState, SnakeCaseToSpaceCase],
declarations: [
ReportsCardComponent,
EllipsisPipe,
HumanizeCurrencyPipe,
ExactCurrencyPipe,
ReportState,
SnakeCaseToSpaceCase,
],
Comment on lines +22 to +29
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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');
});

imports: [IonicModule.forRoot(), MatIconTestingModule, MatIconModule],
providers: [FyCurrencyPipe, CurrencyPipe],
}).compileComponents();
Expand Down
Loading