fix:draft report zero state improvements #3363
Conversation
WalkthroughThe Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
👮 Files not reviewed due to content moderation or server errors (3)
🪧 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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
src/assets/images/zero-states/invoice.pngis excluded by!**/*.png
📒 Files selected for processing (2)
src/app/fyle/my-view-report/my-view-report.page.html(1 hunks)src/app/fyle/my-view-report/my-view-report.page.scss(2 hunks)
| &--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; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mind it! Let's optimize the footer container styles, macha!
The footer container styling needs some improvements:
- Using
z-index: 999is like using a cannon to kill a mosquito! A lower value would suffice. - 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.
| &--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; | |
| } |
| &--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; | ||
| } |
There was a problem hiding this comment.
🧹 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.
| &--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; | |
| } | |
| } |
| <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 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Thalaiva style button improvements needed!
The button section needs some improvements:
- The button text "Add complete expense" is not very clear. Consider "Add existing expense" instead.
- 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;
}| <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> |
There was a problem hiding this comment.
🧹 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.
| <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> |
|
|
||
| &--zero-state-image-container { | ||
| height: 150px; | ||
| width: 150px; |
There was a problem hiding this comment.
would this work for all resolutions of mobile devices?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@Sishhhh, let's share this screenshot with the design team mentioning the resolution.
I think we will need more padding.
| const addExpenseButton = fixture.debugElement.nativeElement.querySelector( | ||
| '.view-reports--footer-conatiner .btn-outline-secondary' | ||
| ) as HTMLElement; |
There was a problem hiding this comment.
🛠️ 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>
|
|
|
||
| &--zero-state-image-container { | ||
| height: 150px; | ||
| width: 150px; |
There was a problem hiding this comment.
@Sishhhh, let's share this screenshot with the design team mentioning the resolution.
I think we will need more padding.
Clickup
https://app.clickup.com/t/86cx8fb73
Code Coverage
Please add code coverage here
UI Preview
Summary by CodeRabbit
New Features
Bug Fixes
Style
Tests