feat: Added the merchant name as Suggestion in expense detail card for CC expenses #3369
Conversation
WalkthroughListen up, superstar! 🌟 This pull request brings a stylish upgrade to the expense management system, introducing a new merchant information feature for corporate card transactions. We've added a sleek popover component that reveals merchant details with a single click, making expense tracking as cool as my signature style. The changes span multiple files, enhancing both the add-edit and view expense pages with a consistent, user-friendly approach. Changes
Possibly related PRs
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
👮 Files not reviewed due to content moderation or server errors (3)
🔇 Additional comments (2)src/app/fyle/add-edit-expense/add-edit-expense-6.spec.ts (2)
The import for CCExpenseMerchantInfoPopoverComponent follows the established pattern and maintains code organization.
The test implementation:
Like my signature moves, this test covers all the right steps! 🪧 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 (
|
| templateUrl: './cc-expense-merchant-info-popover.component.html', | ||
| styleUrls: ['./cc-expense-merchant-info-popover.component.scss'], | ||
| }) | ||
| export class CcExpenseMerchantInfoPopoverComponent { |
There was a problem hiding this comment.
update the name CCExpenseMerchantInfoPopoverComponent
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🔭 Outside diff range comments (1)
src/app/shared/components/transaction-status/transaction-status.component.html (1)
Line range hint
10-16: The click handler needs some keyboard love, machan!Let's make it keyboard-friendly with proper semantic HTML and keyboard support!
-<div (click)="statusClick.emit(transactionStatus)" class="transaction-status__status-value-container"> +<button + (click)="statusClick.emit(transactionStatus)" + (keydown.enter)="statusClick.emit(transactionStatus)" + class="transaction-status__status-value-container" + role="button" + tabindex="0" + aria-label="View transaction status details"> <span class="transaction-status__status-value" data-testid="status-value">{{ transactionStatus | titlecase }}</span> <ion-icon src="../../../assets/svg/info-circle-outline.svg" class="transaction-status__info-icon" data-testid="info-icon" + aria-hidden="true" ></ion-icon> -</div> +</button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
src/app/fyle/add-edit-expense/add-edit-expense.page.html(1 hunks)src/app/fyle/add-edit-expense/add-edit-expense.page.scss(1 hunks)src/app/fyle/add-edit-expense/add-edit-expense.page.ts(2 hunks)src/app/fyle/view-expense/view-expense.page.html(1 hunks)src/app/fyle/view-expense/view-expense.page.scss(1 hunks)src/app/fyle/view-expense/view-expense.page.ts(2 hunks)src/app/shared/components/cc-expense-merchant-info-popover/cc-expense-merchant-info-popover.component.html(1 hunks)src/app/shared/components/cc-expense-merchant-info-popover/cc-expense-merchant-info-popover.component.scss(1 hunks)src/app/shared/components/cc-expense-merchant-info-popover/cc-expense-merchant-info-popover.component.spec.ts(1 hunks)src/app/shared/components/cc-expense-merchant-info-popover/cc-expense-merchant-info-popover.component.ts(1 hunks)src/app/shared/components/transaction-status/transaction-status.component.html(1 hunks)src/app/shared/components/transaction-status/transaction-status.component.scss(1 hunks)src/app/shared/shared.module.ts(3 hunks)
🔇 Additional comments (7)
src/app/shared/components/transaction-status/transaction-status.component.scss (1)
41-41: Superstar styling, perfectly balanced!
The font-size addition maintains visual harmony with the status label. When I make a style statement, it's always consistent!
src/app/fyle/view-expense/view-expense.page.scss (1)
59-86: Mind it! These styles are perfectly structured!
The merchant block styles are well-organized with:
- Proper BEM naming convention
- Consistent use of color variables
- Clean flex layout structure
- Proper spacing and alignment
src/app/fyle/view-expense/view-expense.page.ts (1)
41-41: Clean import, like my entry in a movie!
The component import is properly placed with other imports.
src/app/shared/shared.module.ts (1)
156-156: Listen up, folks! These module changes are spot on!
The CcExpenseMerchantInfoPopoverComponent is properly declared and exported, following the Angular way like a boss! 🎭
Also applies to: 286-286, 422-422
src/app/fyle/view-expense/view-expense.page.html (1)
103-116: Superb structure, partner! This merchant info block is perfectly placed!
The conditional rendering, styling classes, and test attributes are all in perfect harmony! 🌟
src/app/fyle/add-edit-expense/add-edit-expense.page.html (1)
191-204: Magnificent implementation, my friend! The consistency is legendary!
The merchant info block perfectly mirrors the view-expense page implementation, maintaining a consistent user experience across the app! 🌟
src/app/fyle/add-edit-expense/add-edit-expense.page.ts (1)
143-143: Mind it! Import statement looks perfect!
The import for CcExpenseMerchantInfoPopoverComponent follows proper Angular conventions and is well-placed with other component imports.
| .header { | ||
| border-radius: 10px; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mind-it! The header needs more style, machan!
The border-radius of 10px is playing too safe. For a more modern look, let's make it more stylish!
.header {
- border-radius: 10px;
+ border-radius: 16px;
+ overflow: hidden;
}📝 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.
| .header { | |
| border-radius: 10px; | |
| } | |
| .header { | |
| border-radius: 16px; | |
| overflow: hidden; | |
| } |
| &__toolbar-close-btn { | ||
| position: absolute; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Ey! The close button needs some style-u, style-u!
The close button's position is absolute but missing proper coordinates and transitions for smooth animation.
&__toolbar-close-btn {
position: absolute;
+ top: 12px;
+ right: 12px;
+ transition: transform 0.2s ease;
+ &:hover {
+ transform: scale(1.1);
+ }
}📝 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.
| &__toolbar-close-btn { | |
| position: absolute; | |
| } | |
| &__toolbar-close-btn { | |
| position: absolute; | |
| top: 12px; | |
| right: 12px; | |
| transition: transform 0.2s ease; | |
| &:hover { | |
| transform: scale(1.1); | |
| } | |
| } |
| closePopover(): void { | ||
| this.popoverController.dismiss(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
When something goes wrong, handle it with style!
The closePopover method needs proper error handling, just like how I handle villains in my movies!
async closePopover(): Promise<void> {
- this.popoverController.dismiss();
+ try {
+ await this.popoverController.dismiss();
+ } catch (error) {
+ console.error('Failed to dismiss popover:', error);
+ }
}📝 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.
| closePopover(): void { | |
| this.popoverController.dismiss(); | |
| } | |
| async closePopover(): Promise<void> { | |
| try { | |
| await this.popoverController.dismiss(); | |
| } catch (error) { | |
| console.error('Failed to dismiss popover:', error); | |
| } | |
| } |
| export class CcExpenseMerchantInfoPopoverComponent { | ||
| constructor(private popoverController: PopoverController) {} | ||
|
|
There was a problem hiding this comment.
Mind it! Where is the merchant data input, partner?
The component needs to receive merchant information from its parent. Let's add some proper inputs!
export class CcExpenseMerchantInfoPopoverComponent {
+ @Input() merchantName: string;
+ @Input() merchantId: string;
+ @Input() merchantCategory?: string;
constructor(private popoverController: PopoverController) {}Committable suggestion skipped: line range outside the PR's diff.
| @@ -6,8 +6,7 @@ | |||
| posted: transactionStatus === TransactionStatus.POSTED | |||
| }" | |||
| ></div> | |||
|
|
|||
| Transaction Status: | |||
| <div class="transaction-status__status-label">Transaction Status:</div> | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make it accessible for everyone, that's my style!
The transaction status label needs proper accessibility attributes. Let's make it inclusive!
-<div class="transaction-status__status-label">Transaction Status:</div>
+<div class="transaction-status__status-label" aria-label="Transaction Status Label" role="heading">Transaction Status:</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.
| <div class="transaction-status__status-label">Transaction Status:</div> | |
| <div class="transaction-status__status-label" aria-label="Transaction Status Label" role="heading">Transaction Status:</div> |
| &__merchant_block { | ||
| display: flex; | ||
| gap: 6px; | ||
| border-top: 1px solid $grey; | ||
| padding-top: 12px; | ||
| margin-top: 12px; | ||
| align-items: center; | ||
| } | ||
|
|
||
| &__merchant_label { | ||
| color: $black-light; | ||
| font-size: 12px; | ||
| } | ||
|
|
||
| &__merchant_value { | ||
| display: flex; | ||
| align-items: center; | ||
| gap: 2px; | ||
| color: $blue-black; | ||
| font-weight: 500; | ||
| font-size: 12px; | ||
| } | ||
|
|
||
| &__merchant_info_icon { | ||
| color: $blue-black; | ||
| width: 16px; | ||
| height: 16px; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Style pannu, but don't repeat pannu!
These merchant-related styles are duplicated between view-expense and add-edit-expense pages. Let's make it more powerful!
Consider extracting these common styles into a shared SCSS file:
+ // src/theme/shared/_merchant.scss
+ %merchant-block {
+ display: flex;
+ gap: 6px;
+ border-top: 1px solid $grey;
+ padding-top: 12px;
+ margin-top: 12px;
+ align-items: center;
+ }
+
+ %merchant-label {
+ color: $black-light;
+ font-size: 12px;
+ }
+
+ %merchant-value {
+ display: flex;
+ align-items: center;
+ gap: 2px;
+ color: $blue-black;
+ font-weight: 500;
+ font-size: 12px;
+ }
+
+ %merchant-info-icon {
+ color: $blue-black;
+ width: 16px;
+ height: 16px;
+ }Then in both SCSS files:
+ @import '../../../theme/shared/merchant';
&__merchant_block {
- display: flex;
- gap: 6px;
- border-top: 1px solid $grey;
- padding-top: 12px;
- margin-top: 12px;
- align-items: center;
+ @extend %merchant-block;
}
// Similarly extend other merchant-related stylesCommittable suggestion skipped: line range outside the PR's diff.
| async openCCExpenseMerchantInfoModal(): Promise<void> { | ||
| const popover = await this.popoverController.create({ | ||
| component: CcExpenseMerchantInfoPopoverComponent, | ||
| cssClass: 'fy-dialog-popover', | ||
| }); | ||
|
|
||
| await popover.present(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
The modal needs some error handling power!
The modal presentation needs to handle potential errors, just like how I handle villains in my movies!
Add error handling:
async openCCExpenseMerchantInfoModal(): Promise<void> {
+ try {
const popover = await this.popoverController.create({
component: CcExpenseMerchantInfoPopoverComponent,
cssClass: 'fy-dialog-popover',
});
await popover.present();
+ } catch (error) {
+ console.error('Failed to present merchant info modal:', error);
+ // Consider showing a user-friendly error message
+ throw error;
+ }
}📝 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.
| async openCCExpenseMerchantInfoModal(): Promise<void> { | |
| const popover = await this.popoverController.create({ | |
| component: CcExpenseMerchantInfoPopoverComponent, | |
| cssClass: 'fy-dialog-popover', | |
| }); | |
| await popover.present(); | |
| } | |
| async openCCExpenseMerchantInfoModal(): Promise<void> { | |
| try { | |
| const popover = await this.popoverController.create({ | |
| component: CcExpenseMerchantInfoPopoverComponent, | |
| cssClass: 'fy-dialog-popover', | |
| }); | |
| await popover.present(); | |
| } catch (error) { | |
| console.error('Failed to present merchant info modal:', error); | |
| // Consider showing a user-friendly error message | |
| throw error; | |
| } | |
| } |
| (click)="(openCCExpenseMerchantInfoModal())" | ||
| src="../../../assets/svg/info-circle-outline.svg" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Style, my friend! Let's make that click handler more stylish!
Remove those extra parentheses around the click handler - they're not needed! 🎬
- (click)="(openCCExpenseMerchantInfoModal())"
+ (click)="openCCExpenseMerchantInfoModal()"📝 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.
| (click)="(openCCExpenseMerchantInfoModal())" | |
| src="../../../assets/svg/info-circle-outline.svg" | |
| (click)="openCCExpenseMerchantInfoModal()" | |
| src="../../../assets/svg/info-circle-outline.svg" |
There was a problem hiding this comment.
@Chethan-Fyle, thank you for your feedback, boss! Much appreciated!
╔(_/)
║( •~•) ← So happy, kabali style!
╚(⌒⌒)づ
| (click)="(openCCExpenseMerchantInfoModal())" | ||
| src="../../../assets/svg/info-circle-outline.svg" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Style encore! Same click handler needs the same style fix!
Let's keep our event handlers clean and crisp! 🎭
- (click)="(openCCExpenseMerchantInfoModal())"
+ (click)="openCCExpenseMerchantInfoModal()"📝 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.
| (click)="(openCCExpenseMerchantInfoModal())" | |
| src="../../../assets/svg/info-circle-outline.svg" | |
| (click)="openCCExpenseMerchantInfoModal()" | |
| src="../../../assets/svg/info-circle-outline.svg" |
| async openCCExpenseMerchantInfoModal(): Promise<void> { | ||
| const popover = await this.popoverController.create({ | ||
| component: CcExpenseMerchantInfoPopoverComponent, | ||
| cssClass: 'fy-dialog-popover', | ||
| }); | ||
|
|
||
| await popover.present(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Magizhchi! The implementation looks good, but let's add error handling!
The popover implementation follows good practices, but could be more robust with error handling. Here's a suggested improvement:
async openCCExpenseMerchantInfoModal(): Promise<void> {
+ try {
const popover = await this.popoverController.create({
component: CcExpenseMerchantInfoPopoverComponent,
cssClass: 'fy-dialog-popover',
});
await popover.present();
+ } catch (error) {
+ console.error('Failed to present merchant info popover:', error);
+ // Handle the error appropriately, maybe show a fallback UI
+ }
}📝 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.
| async openCCExpenseMerchantInfoModal(): Promise<void> { | |
| const popover = await this.popoverController.create({ | |
| component: CcExpenseMerchantInfoPopoverComponent, | |
| cssClass: 'fy-dialog-popover', | |
| }); | |
| await popover.present(); | |
| } | |
| async openCCExpenseMerchantInfoModal(): Promise<void> { | |
| try { | |
| const popover = await this.popoverController.create({ | |
| component: CcExpenseMerchantInfoPopoverComponent, | |
| cssClass: 'fy-dialog-popover', | |
| }); | |
| await popover.present(); | |
| } catch (error) { | |
| console.error('Failed to present merchant info popover:', error); | |
| // Handle the error appropriately, maybe show a fallback UI | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/app/fyle/add-edit-expense/add-edit-expense.page.html(1 hunks)src/app/fyle/add-edit-expense/add-edit-expense.page.ts(2 hunks)src/app/fyle/view-expense/view-expense.page.html(1 hunks)src/app/fyle/view-expense/view-expense.page.ts(2 hunks)src/app/shared/components/cc-expense-merchant-info-popover/cc-expense-merchant-info-popover.component.ts(1 hunks)src/app/shared/shared.module.ts(3 hunks)
👮 Files not reviewed due to content moderation or server errors (2)
- src/app/fyle/add-edit-expense/add-edit-expense.page.html
- src/app/fyle/add-edit-expense/add-edit-expense.page.ts
🔇 Additional comments (6)
src/app/fyle/view-expense/view-expense.page.html (2)
109-109: Style, my friend! Let's make that click handler more stylish!
Remove those extra parentheses around the click handler - they're not needed! 🎬
-(click)="openCCExpenseMerchantInfoModal()"
+(click)="openCCExpenseMerchantInfoModal()"103-116: The merchant block looks perfect, thalaiva! 🌟
The implementation aligns well with the PR objective of adding merchant name suggestions in the expense detail card. The conditional rendering, fallback text, and info icon integration are all properly implemented.
src/app/shared/components/cc-expense-merchant-info-popover/cc-expense-merchant-info-popover.component.ts (2)
9-11:
Ey! Where is the merchant data input, macha?
Just like how I never enter a scene without my signature style, this component needs its essential inputs!
export class CCExpenseMerchantInfoPopoverComponent {
+ @Input() merchantName: string;
+ @Input() merchantId: string;
+ @Input() merchantCategory?: string;
constructor(private popoverController: PopoverController) {}Likely invalid or redundant comment.
12-14:
Mind it! Your error handling needs some style, partner!
The closePopover method needs proper error handling, just like how I handle my stunts - with precision and safety!
- closePopover(): void {
- this.popoverController.dismiss();
+ async closePopover(): Promise<void> {
+ try {
+ await this.popoverController.dismiss();
+ } catch (error) {
+ console.error('Failed to dismiss popover:', error);
+ }
+ }Likely invalid or redundant comment.
src/app/fyle/view-expense/view-expense.page.ts (1)
520-527:
Style ah? Where is the error handling, thalaiva?
The modal presentation needs proper error handling and merchant data passing, just like how I handle my movie climax - with perfection!
async openCCExpenseMerchantInfoModal(): Promise<void> {
+ try {
const popover = await this.popoverController.create({
component: CCExpenseMerchantInfoPopoverComponent,
cssClass: 'fy-dialog-popover',
+ componentProps: {
+ merchantName: this.expense?.matched_corporate_card_transactions?.[0]?.merchant,
+ merchantId: this.expense?.matched_corporate_card_transactions?.[0]?.merchant_id,
+ merchantCategory: this.expense?.matched_corporate_card_transactions?.[0]?.merchant_category
+ }
});
await popover.present();
+ } catch (error) {
+ console.error('Failed to present merchant info modal:', error);
+ throw error;
+ }
}Likely invalid or redundant comment.
src/app/shared/shared.module.ts (1)
286-286: Superstar style! The module changes are perfect!
The component is properly declared and exported in the SharedModule. Mind-blowing implementation, just like my signature moves!
Also applies to: 422-422
| <ion-icon | ||
| (click)="openCCExpenseMerchantInfoModal()" | ||
| src="../../../assets/svg/info-circle-outline.svg" | ||
| class="view-expense--card-container__merchant_info_icon" | ||
| data-testid="merchant-info-icon" | ||
| ></ion-icon> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Listen up, macha! Let's make our app accessible to everyone!
The info icon needs an aria-label to help screen readers understand its purpose. Also, add a tooltip to provide visual hint to users.
<ion-icon
(click)="openCCExpenseMerchantInfoModal()"
src="../../../assets/svg/info-circle-outline.svg"
class="view-expense--card-container__merchant_info_icon"
data-testid="merchant-info-icon"
+ aria-label="View merchant information"
+ matTooltip="Click for more details about the merchant"
></ion-icon>📝 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.
| <ion-icon | |
| (click)="openCCExpenseMerchantInfoModal()" | |
| src="../../../assets/svg/info-circle-outline.svg" | |
| class="view-expense--card-container__merchant_info_icon" | |
| data-testid="merchant-info-icon" | |
| ></ion-icon> | |
| <ion-icon | |
| (click)="openCCExpenseMerchantInfoModal()" | |
| src="../../../assets/svg/info-circle-outline.svg" | |
| class="view-expense--card-container__merchant_info_icon" | |
| data-testid="merchant-info-icon" | |
| aria-label="View merchant information" | |
| matTooltip="Click for more details about the merchant" | |
| ></ion-icon> |
|
Clickup
https://app.clickup.com/t/86cxbrpvr
UI Preview
Summary by CodeRabbit
New Features
Bug Fixes
Style
Tests