Fix: fixed the zero state expense #3359
Conversation
palgunams1603
requested review from
Chethan-Fyle and
arjunaj5
as code owners
WalkthroughSuperstar style, I'll break down these changes like a mass hero! 🌟 The pull request focuses on enhancing the my-expenses page's user interface, introducing smart zero-state handling and improved loading experiences. The modifications refine how the application displays expenses, adding intelligent conditional rendering that adapts to user connectivity and expense availability. It's all about making the user interface as stylish as a Rajinikanth entry scene - smooth, responsive, and totally epic! Changes
Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
src/app/fyle/my-expenses/my-expenses.page.html (3)
Line range hint
134-148: Mind it! Let's make this zero state as inclusive as my fan following! 🎭The zero state looks stylish, but let's add some accessibility improvements to make it legendary!
Add these aria labels to make it more accessible:
<div *ngIf="(pendingTransactions.length === 0) && ((count$ | async) === 0) && isConnected$|async" - class="my-expenses--zero-state" + class="my-expenses--zero-state" + role="status" + aria-live="polite" > <img alt="Create First Expenses" class="my-expenses--zero-state-img" src="../../../assets/images/zero-states/expenses.png" + aria-hidden="true" />
Line range hint
201-211: Time to make this scroll smoother than my signature walk! 🚶♂️The infinite scroll implementation is good, but let's optimize it like a well-choreographed action sequence!
Consider using virtual scrolling for better performance with large lists:
+<cdk-virtual-scroll-viewport itemSize="50" class="my-expenses--virtual-scroll"> <ng-container *ngIf="isConnected$|async"> <ng-container *ngIf="!isLoadingDataInInfiniteScroll"> <ion-infinite-scroll (ionInfinite)="loadData($event)" *ngIf="isInfiniteScrollRequired$ | async" threshold="100px"> <ion-infinite-scroll-content></ion-infinite-scroll-content> </ion-infinite-scroll> </ng-container> </ng-container> +</cdk-virtual-scroll-viewport>Don't forget to style it in your SCSS:
.my-expenses--virtual-scroll { height: 100%; width: 100%; }
Mind it! Your template needs some performance style, macha! 🕶️
Listen up, thalaiva! I found some performance concerns that need our signature style:
- Multiple async pipes without trackBy functions in ngFor loops - this is like doing a mass scene without proper choreography!
- No OnPush change detection strategy - we're missing the punch dialogues of performance optimization
- Two separate ngFor loops rendering similar expense cards - that's like having two heroes in one scene!
Quick fixes to make this template a blockbuster:
- Add trackBy functions to both ngFor loops
- Implement OnPush change detection
- Consider combining the pending and connected expense lists into a single stream
Remember what I always say: "When performance comes in style, success comes in smile!"
🔗 Analysis chain
Line range hint
1-211: Let's verify this performance like we check the timing of a perfect stunt! 🎬We should measure the impact of these template changes on the rendering performance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential performance bottlenecks in templates # Look for multiple async pipe usage that might cause unnecessary change detection rg -l "async.*async" --type html # Check for large templates that might benefit from breaking into components find . -name "*.html" -exec wc -l {} \; | sort -nr | head -n 5Length of output: 985
Script:
#!/bin/bash # Let's check for potential performance bottlenecks in my-expenses.page.html # Check for ngFor with async pipe combinations which could cause performance issues rg "ngFor.*async" src/app/fyle/my-expenses/my-expenses.page.html -A 2 # Check for trackBy usage in ngFor loops rg "ngFor" src/app/fyle/my-expenses/my-expenses.page.html # Check if OnPush change detection is used in the component rg "changeDetection" src/app/fyle/my-expenses/my-expenses.page.ts # Look for any existing performance optimizations ast-grep --pattern 'class MyExpensesPage implements $$$'Length of output: 562
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/app/fyle/my-expenses/my-expenses.page.html(1 hunks)src/app/fyle/my-expenses/my-expenses.page.scss(2 hunks)
🔇 Additional comments (2)
src/app/fyle/my-expenses/my-expenses.page.scss (2)
54-56: Mind it! The zero state padding looks perfect, boss!
The new class removes bottom padding when there are no expenses to show. Clean and precise like my signature move! 🕶️
287-287: Kabali da! The spacing adjustment is spot on!
Adding 32px padding-top gives the zero state content the perfect stage presence. When combined with height: 100%, it creates a balanced layout that even my style would approve!
| <ion-content | ||
| [ngClass]="{ | ||
| 'd-none': isCameraPreviewStarted, | ||
| 'my-expenses--content-zero-state': ((pendingTransactions.length === 0) && ((count$ | async) === 0) && isConnected$ | async) | ||
| }" | ||
| class="my-expenses--content" | ||
| (click)="onPageClick()" | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Listen here, partner! Let's make this template as powerful as a punch dialogue! 🌟
The template condition is getting more complex than my signature sunglasses collection! We should handle this logic in the component.
Here's a style that would make even the bad guys jealous:
-[ngClass]="{
- 'd-none': isCameraPreviewStarted,
- 'my-expenses--content-zero-state': ((pendingTransactions.length === 0) && ((count$ | async) === 0) && isConnected$ | async)
-}"
+[ngClass]="{
+ 'd-none': isCameraPreviewStarted,
+ 'my-expenses--content-zero-state': shouldShowZeroState$ | async
+}"And in your component:
shouldShowZeroState$ = combineLatest([
this.count$,
this.isConnected$
]).pipe(
map(([count, isConnected]) =>
this.pendingTransactions.length === 0 && count === 0 && isConnected
)
);| @@ -91,7 +91,14 @@ | |||
| </ng-container> | |||
There was a problem hiding this comment.
Please share before and after screenshots.
|
UI Preview
Before
After
Clickup
https://app.clickup.com/t/86cxbr6w1
Summary by CodeRabbit
New Features
Style