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

fix:draft report zero state improvements #3363

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+45 −11
Open

fix:draft report zero state improvements #3363

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a8316dc
fix:draft report zero state improvements
Dec 17, 2024
2562483
fixing unit test error
Dec 19, 2024
a5f53d0
fixing unit test
Dec 19, 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
26 changes: 16 additions & 10 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 @@ -170,21 +170,27 @@
</div>
<div class="view-reports--expenses-zero-state" *ngIf="!isExpensesLoading && report.num_expenses === 0">
<div class="text-center">
<img src="../../../assets/images/zero-states/expenses.png" alt="No expense in this report" />
<div class="view-reports--zero-state-header">Looks like this report is empty.</div>
<div class="view-reports--zero-state-image-container">
<img src="../../../assets/images/zero-states/invoice.png" alt="No expense in this report" />
</div>
<div class="view-reports--zero-state-semibold-header">Looks like you’ve got</div>
<div class="view-reports--zero-state-header">0 expense right now</div>
</div>
Comment on lines +173 to +178
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 zero state looks good, but let's make it perfect!

The alt text could be more descriptive for better accessibility.

- <img src="../../../assets/images/zero-states/invoice.png" alt="No expense in this report" />
+ <img src="../../../assets/images/zero-states/invoice.png" alt="Empty report state - No expenses have been added to this report yet" />
📝 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
<div class="view-reports--zero-state-image-container">
<img src="../../../assets/images/zero-states/invoice.png" alt="No expense in this report" />
</div>
<div class="view-reports--zero-state-semibold-header">Looks like you’ve got</div>
<div class="view-reports--zero-state-header">0 expense right now</div>
</div>
<div class="view-reports--zero-state-image-container">
<img src="../../../assets/images/zero-states/invoice.png" alt="Empty report state - No expenses have been added to this report yet" />
</div>
<div class="view-reports--zero-state-semibold-header">Looks like you've got</div>
<div class="view-reports--zero-state-header">0 expense right now</div>
</div>

<div class="view-reports--footer-conatiner">
<button
*ngIf="unreportedExpenses && unreportedExpenses.length > 0"
class="view-reports--unreported-cta"
class="btn-primary"
(click)="showAddExpensesToReportModal()"
>
Add complete expenses
Add complete expense
</button>
<button
class="btn-outline-secondary"
[ngClass]="{'btn-primary': !(unreportedExpenses && unreportedExpenses.length > 0), 'btn-outline-secondary': unreportedExpenses && unreportedExpenses.length > 0}"
(click)="addExpense()"
>
Add new expense
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Thalaiva style button improvements needed!

The button section needs some improvements:

  1. The button text "Add complete expense" is not very clear. Consider "Add existing expense" instead.
  2. The ngClass condition can be simplified.
- <button
-   class="btn-outline-secondary"
-   [ngClass]="{'btn-primary': !(unreportedExpenses && unreportedExpenses.length > 0), 'btn-outline-secondary': unreportedExpenses && unreportedExpenses.length > 0}"
-   (click)="addExpense()"
- >
-   Add new expense
- </button>
+ <button
+   [class]="unreportedExpenses?.length ? 'btn-outline-secondary' : 'btn-primary'"
+   (click)="addExpense()"
+ >
+   Create new expense
+ </button>

Also, consider extracting the unreportedExpenses check to a getter in the component:

get hasUnreportedExpenses(): boolean {
  return this.unreportedExpenses?.length > 0;
}

</button>
<div class="view-reports--add-more-container" (click)="addExpense()">
<div class="view-reports--add-more-icon-container">
<mat-icon class="view-reports--add-more-icon" svgIcon="plus-square"></mat-icon>
</div>
<div class="view-reports--add-more-text">Add Expenses</div>
</div>
</div>
</div>
</ng-container>
Expand Down
27 changes: 27 additions & 0 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 @@ -56,6 +56,19 @@
}
}

&--footer-conatiner {
display: flex;
justify-content: space-between;
padding: 16px;
gap: 8px;
position: fixed;
bottom: 0;
Sishhhh marked this conversation as resolved.
Show resolved Hide resolved
width: 100%;
background-color: $pure-white;
box-shadow: 0 -2px 12px $pink-shadow;
Sishhhh marked this conversation as resolved.
Show resolved Hide resolved
z-index: 999;
}
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Mind it! Let's optimize the footer container styles, macha!

The footer container styling needs some improvements:

  1. Using z-index: 999 is like using a cannon to kill a mosquito! A lower value would suffice.
  2. Fixed positioning with 100% width might cause issues on smaller screens.
  &--footer-conatiner {
    display: flex;
    justify-content: space-between;
    padding: 16px;
    gap: 8px;
    position: fixed;
    bottom: 0;
    width: 100%;
    background-color: $pure-white;
    box-shadow: 0 -2px 12px $pink-shadow;
-   z-index: 999;
+   z-index: 10;
+   max-width: 100vw;
+   box-sizing: border-box;
  }
📝 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
&--footer-conatiner {
display: flex;
justify-content: space-between;
padding: 16px;
gap: 8px;
position: fixed;
bottom: 0;
width: 100%;
background-color: $pure-white;
box-shadow: 0 -2px 12px $pink-shadow;
z-index: 999;
}
&--footer-conatiner {
display: flex;
justify-content: space-between;
padding: 16px;
gap: 8px;
position: fixed;
bottom: 0;
width: 100%;
background-color: $pure-white;
box-shadow: 0 -2px 12px $pink-shadow;
z-index: 10;
max-width: 100vw;
box-sizing: border-box;
}


&--card-body {
font-size: 16px;
color: $gray-3;
Expand Down Expand Up @@ -350,6 +363,20 @@
font-weight: 500;
}

&--zero-state-semibold-header {
font-size: 14px;
display: block;
color: $blue-black;
line-height: 1.5;
font-weight: 400;
}

&--zero-state-image-container {
height: 150px;
width: 150px;
Copy link
Contributor

Choose a reason for hiding this comment

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

would this work for all resolutions of mobile devices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arjunaj5 I have checked it across multiple dimensions, and I think it looks good. At a dimension of 320px, it appears fine as well. Attaching a photo for reference.
Screenshot 2024-12-20 at 5 54 16 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

@Sishhhh, let's share this screenshot with the design team mentioning the resolution.
I think we will need more padding.

overflow: hidden;
}
Comment on lines +366 to +378
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 pandratha style-a pandrom! (Let's style it with style!)

The zero state image container needs to be more responsive:

  &--zero-state-image-container {
-   height: 150px;
-   width: 150px;
+   height: min(150px, 30vh);
+   width: min(150px, 90%);
    overflow: hidden;
+   margin: 0 auto;
+   display: flex;
+   align-items: center;
+   justify-content: center;
+   
+   img {
+     max-width: 100%;
+     height: 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.

Suggested change
&--zero-state-semibold-header {
font-size: 14px;
display: block;
color: $blue-black;
line-height: 1.5;
font-weight: 400;
}
&--zero-state-image-container {
height: 150px;
width: 150px;
overflow: hidden;
}
&--zero-state-semibold-header {
font-size: 14px;
display: block;
color: $blue-black;
line-height: 1.5;
font-weight: 400;
}
&--zero-state-image-container {
height: min(150px, 30vh);
width: min(150px, 90%);
overflow: hidden;
margin: 0 auto;
display: flex;
align-items: center;
justify-content: center;
img {
max-width: 100%;
height: auto;
}
}


&--history-container {
background: $pure-white;
border-radius: 16px;
Expand Down
4 changes: 3 additions & 1 deletion 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 @@ -930,7 +930,9 @@ describe('MyViewReportPage', () => {
component.report$ = of(cloneDeep({ ...platformReportData, state: 'DRAFT' }));
fixture.detectChanges();

const addExpenseButton = getElementBySelector(fixture, '.view-reports--add-more-container') as HTMLElement;
const addExpenseButton = fixture.debugElement.nativeElement.querySelector(
'.view-reports--footer-conatiner .btn-outline-secondary'
) as HTMLElement;
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Mind-blowing suggestion: Use data-testid for more stable test selectors!

Hey there! Like a perfectly choreographed action sequence, your tests should be robust and reliable! Instead of relying on complex class-based selectors that can break when styles change, let's add a data-testid attribute to make our test selector more stable, thalaiva style!

-    const addExpenseButton = fixture.debugElement.nativeElement.querySelector(
-      '.view-reports--footer-conatiner .btn-outline-secondary'
-    ) as HTMLElement;
+    const addExpenseButton = fixture.debugElement.nativeElement.querySelector(
+      '[data-testid="add-expense-button"]'
+    ) as HTMLElement;

Don't forget to add the corresponding data-testid attribute to your component template:

<button data-testid="add-expense-button" class="view-reports--footer-conatiner btn-outline-secondary">
  Add Expense
</button>

click(addExpenseButton);

expect(router.navigate).toHaveBeenCalledOnceWith([
Expand Down
Binary file added src/assets/images/zero-states/invoice.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading