-
Notifications
You must be signed in to change notification settings - Fork 15
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.
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: add new review-split-expense template #3463
base: feat_split_expense
Are you sure you want to change the base?
Conversation
WalkthroughThis pull request introduces a new Angular component named Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as ReviewSplitExpenseComponent
participant M as ModalController
U->>C: Click on expense card (goToTransaction)
C->>M: dismiss({ action: "goToTransaction", expense: selectedExpense })
M-->>C: Modal dismissed
U->>C: Click on close button
C->>M: dismiss({ action: "close" })
M-->>C: Modal dismissed
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
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 (
|
PR description must contain a link to a ClickUp (case-insensitive) |
PR description must contain a link to a ClickUp (case-insensitive) |
PR description must contain a link to a ClickUp (case-insensitive) |
PR description must contain a link to a ClickUp (case-insensitive) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/app/fyle/split-expense/split-expense.page.spec.ts
(1 hunks)src/app/fyle/split-expense/split-expense.page.ts
(4 hunks)src/app/shared/components/review-split-expense/review-split-expense.component.html
(1 hunks)src/app/shared/components/review-split-expense/review-split-expense.component.scss
(1 hunks)src/app/shared/components/review-split-expense/review-split-expense.component.spec.ts
(1 hunks)src/app/shared/components/review-split-expense/review-split-expense.component.ts
(1 hunks)src/app/shared/shared.module.ts
(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/fyle/split-expense/split-expense.page.ts
[error] 1141-1141: This let declares a variable that is only assigned once.
'category' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
🪛 ESLint
src/app/fyle/split-expense/split-expense.page.ts
[error] 1141-1141: 'category' is never reassigned. Use 'const' instead.
(prefer-const)
🪛 GitHub Check: Run linters
src/app/fyle/split-expense/split-expense.page.ts
[failure] 1121-1121:
Unsafe assignment of an any
value
[failure] 1130-1130:
Unsafe array destructuring of a tuple element with an any
value
[failure] 1133-1133:
Unsafe member access .action on an any
value
[failure] 1139-1139:
Unsafe member access .action on an any
value
[failure] 1140-1140:
Unsafe assignment of an any
value
[failure] 1140-1140:
Unsafe member access .expense on an any
value
[failure] 1141-1141:
Unsafe assignment of an any
value
[failure] 1141-1141:
'category' is never reassigned. Use 'const' instead
[failure] 1141-1141:
Unsafe member access .category on an any
value
[failure] 1141-1141:
Unsafe call of an any
typed value
🪛 GitHub Actions: Lint
src/app/fyle/split-expense/split-expense.page.ts
[error] 1121-1121: Unsafe assignment of an any
value
🔇 Additional comments (1)
src/app/fyle/split-expense/split-expense.page.spec.ts (1)
1949-1967
: Hey, why you skipping this important test case, partner?The test case for handling API failure in
postSplitExpenseComments
is marked as skipped. This test is crucial for ensuring proper error handling when the API fails.Please enable this test case by removing the
x
prefix fromxit
to ensure robust error handling coverage.
src/app/shared/components/review-split-expense/review-split-expense.component.ts
Outdated
Show resolved
Hide resolved
src/app/shared/components/review-split-expense/review-split-expense.component.ts
Outdated
Show resolved
Hide resolved
src/app/shared/components/review-split-expense/review-split-expense.component.spec.ts
Outdated
Show resolved
Hide resolved
@@ -249,6 +250,7 @@ import { CCExpenseMerchantInfoModalComponent } from './components/cc-expense-mer | |||
FyPolicyViolationComponent, | |||
PolicyViolationActionComponent, | |||
SplitExpensePolicyViolationComponent, | |||
ReviewSplitExpenseComponent, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Hey thalaiva, don't forget to export your powerful component!
The ReviewSplitExpenseComponent needs to be added to the exports array if it will be used in other modules.
Add the component to the exports array:
exports: [
+ ReviewSplitExpenseComponent,
// ... other exports
],
📝 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.
ReviewSplitExpenseComponent, | |
@NgModule({ | |
declarations: [ | |
// ... other declarations | |
ReviewSplitExpenseComponent, | |
// ... possibly more declarations | |
], | |
exports: [ | |
ReviewSplitExpenseComponent, | |
// ... other exports | |
], | |
imports: [ | |
// ... imports | |
] | |
}) | |
export class SharedModule { } |
src/app/shared/components/review-split-expense/review-split-expense.component.scss
Outdated
Show resolved
Hide resolved
src/app/shared/components/review-split-expense/review-split-expense.component.html
Outdated
Show resolved
Hide resolved
src/app/shared/components/review-split-expense/review-split-expense.component.html
Outdated
Show resolved
Hide resolved
PR description must contain a link to a ClickUp (case-insensitive) |
1 similar comment
PR description must contain a link to a ClickUp (case-insensitive) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/app/fyle/split-expense/split-expense.page.ts
(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/fyle/split-expense/split-expense.page.ts
[error] 1143-1143: This let declares a variable that is only assigned once.
'category' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
🪛 ESLint
src/app/fyle/split-expense/split-expense.page.ts
[error] 1143-1143: 'category' is never reassigned. Use 'const' instead.
(prefer-const)
🪛 GitHub Check: Run linters
src/app/fyle/split-expense/split-expense.page.ts
[failure] 1143-1143:
Unsafe assignment of an any
value
[failure] 1143-1143:
'category' is never reassigned. Use 'const' instead
[failure] 1143-1143:
Unsafe member access .name on an any
value
[failure] 1143-1143:
Unsafe call of an any
typed value
[failure] 1146-1146:
Unsafe assignment of an any
value
[failure] 1148-1148:
Unsafe assignment of an any
value
[failure] 1150-1150:
Unsafe assignment of an any
value
🪛 GitHub Actions: Lint
src/app/fyle/split-expense/split-expense.page.ts
[error] 1143-1143: Unsafe assignment of an any
value
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (12.x)
🔇 Additional comments (3)
src/app/fyle/split-expense/split-expense.page.ts (3)
53-54
: Mind-blowing imports, thalaiva!The imports are perfectly aligned with the new modal functionality.
699-699
: Style ah irukku, thalaiva! Perfect timing for the modal!The modal is shown right after the split expense operation succeeds, providing a smooth user experience.
1118-1154
: 🛠️ Refactor suggestionTime to add some superhero-level type safety to your code, thalaiva!
Let's make the code more robust with proper TypeScript types:
- async openReviewSplitExpenseModal(expense: Expense): Promise<void> { + async openReviewSplitExpenseModal(expense: PlatformExpense[]): Promise<void> { const reviewModal = await this.modalController.create({ component: ReviewSplitExpenseComponent, componentProps: { - splitExpenses: expense, + splitExpenses: expense as Expense[], }, mode: 'ios', presentingElement: await this.modalController.getTop(), ...this.modalProperties.getModalDefaultProperties(), }); await reviewModal.present(); - const { data }: { data?: { dismissed: boolean; action: string; expense?: Expense } } = + interface ModalData { + action: 'close' | 'navigate'; + expense?: Expense; + } + + const { data } = await reviewModal.onWillDismiss<ModalData>();Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 1143-1143: This let declares a variable that is only assigned once.
'category' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
🪛 ESLint
[error] 1143-1143: 'category' is never reassigned. Use 'const' instead.
(prefer-const)
🪛 GitHub Check: Run linters
[failure] 1143-1143:
Unsafe assignment of anany
value
[failure] 1143-1143:
'category' is never reassigned. Use 'const' instead
[failure] 1143-1143:
Unsafe member access .name on anany
value
[failure] 1143-1143:
Unsafe call of anany
typed value
[failure] 1146-1146:
Unsafe assignment of anany
value
[failure] 1148-1148:
Unsafe assignment of anany
value
[failure] 1150-1150:
Unsafe assignment of anany
value🪛 GitHub Actions: Lint
[error] 1143-1143: Unsafe assignment of an
any
value
PR description must contain a link to a ClickUp (case-insensitive) |
PR description must contain a link to a ClickUp (case-insensitive) |
PR description must contain a link to a ClickUp (case-insensitive) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/app/shared/components/review-split-expense/review-split-expense.component.html (1)
16-25
: 🧹 Nitpick (assertive)Expense List – Time to supercharge your iteration!
My friend, while your expense list is working fine, the legendary coder in you can take it one step further by switching to semantic elements like<ul>
and<li>
and by using atrackBy
function for thengFor
loop. This not only boosts performance but also adds clarity – a true signature move. Note: This suggestion echoes previous feedback, so it’s tagged as duplicate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/app/shared/components/review-split-expense/review-split-expense.component.html
(1 hunks)src/app/shared/components/review-split-expense/review-split-expense.component.scss
(1 hunks)src/app/shared/components/review-split-expense/review-split-expense.component.spec.ts
(1 hunks)src/app/shared/components/review-split-expense/review-split-expense.component.ts
(1 hunks)src/app/shared/shared.module.ts
(3 hunks)src/global.scss
(1 hunks)
🔇 Additional comments (4)
src/app/shared/components/review-split-expense/review-split-expense.component.ts (2)
10-10
: Mind-blowing input property, macha! But let's add some type safety!The input property needs proper typing to ensure type safety.
- @Input() splitExpenses: Expense[]; + @Input() splitExpenses!: Expense[];
14-20
: Your code style is as powerful as my punch dialogues, but let's make it even stronger!The event parameter needs proper typing for better type safety and maintainability.
- goToTransaction(event: { expense: Expense; expenseIndex: number }): void { + interface TransactionEvent { + expense: Expense; + expenseIndex: number; + } + + goToTransaction(event: TransactionEvent): void {src/app/shared/shared.module.ts (1)
120-120
: Your module changes are as perfect as my style, macha!The component is properly declared and exported in the shared module.
Also applies to: 253-253, 425-425
src/app/shared/components/review-split-expense/review-split-expense.component.scss (1)
3-27
: 🧹 Nitpick (assertive)Your styles are stylish, but let's make them legendary!
Consider using CSS custom properties for better maintainability.
.review-split-expense-modal { + --review-modal-font-size: 18px; + --review-modal-line-height: 27px; + --review-modal-padding: 12px; + --review-modal-margin: 8px; border-bottom: 1px solid $grey; - margin-bottom: 8px; + margin-bottom: var(--review-modal-margin); &--header { display: flex; justify-content: flex-start; align-items: center; - font-size: 18px; + font-size: var(--review-modal-font-size); font-weight: 500; - line-height: 27px; + line-height: var(--review-modal-line-height); - padding-inline: 12px; + padding-inline: var(--review-modal-padding); }Likely invalid or redundant comment.
src/app/shared/components/review-split-expense/review-split-expense.component.spec.ts
Outdated
Show resolved
Hide resolved
src/app/shared/components/review-split-expense/review-split-expense.component.scss
Outdated
Show resolved
Hide resolved
src/app/shared/components/review-split-expense/review-split-expense.component.html
Show resolved
Hide resolved
src/app/shared/components/review-split-expense/review-split-expense.component.html
Outdated
Show resolved
Hide resolved
ion-modal.review-split-expense-modal { | ||
&::part(content) { | ||
border-radius: 0 !important; | ||
max-height: 100% !important; | ||
position: absolute; | ||
top: 0 !important; | ||
bottom: 0 !important; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Modal Style – A blockbuster design!
My friend, the new styles for ion-modal.review-split-expense-modal
are absolutely superstar—ensuring the modal spans the full height of the screen. Just double-check to ensure these overrides (with !important
) play well with other modals in the app. Small tweaks now could save troubles later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sishhhh, can you check why there's some space at the bottom?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dimple, that was due to the showBackDrop set as true when we opened the modal from the split-expense page. Setting it to false
will remove the extra space from the bottom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added comments
src/app/shared/components/review-split-expense/review-split-expense.component.html
Outdated
Show resolved
Hide resolved
src/app/shared/components/review-split-expense/review-split-expense.component.scss
Outdated
Show resolved
Hide resolved
src/app/shared/components/review-split-expense/review-split-expense.component.scss
Outdated
Show resolved
Hide resolved
src/app/shared/components/review-split-expense/review-split-expense.component.scss
Outdated
Show resolved
Hide resolved
ion-modal.review-split-expense-modal { | ||
&::part(content) { | ||
border-radius: 0 !important; | ||
max-height: 100% !important; | ||
position: absolute; | ||
top: 0 !important; | ||
bottom: 0 !important; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sishhhh, can you check why there's some space at the bottom?
src/app/shared/components/review-split-expense/review-split-expense.component.ts
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,60 @@ | |||
import { ComponentFixture, TestBed, waitForAsync } from '@angular/core/testing'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the screenshot of the code coverage report?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/app/shared/components/review-split-expense/review-split-expense.component.spec.ts
Show resolved
Hide resolved
src/app/shared/components/review-split-expense/review-split-expense.component.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/app/shared/components/review-split-expense/review-split-expense.component.html
(1 hunks)src/app/shared/components/review-split-expense/review-split-expense.component.scss
(1 hunks)src/app/shared/components/review-split-expense/review-split-expense.component.spec.ts
(1 hunks)src/app/shared/components/review-split-expense/review-split-expense.component.ts
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (12.x)
🔇 Additional comments (1)
src/app/shared/components/review-split-expense/review-split-expense.component.html (1)
1-12
: 🧹 Nitpick (assertive)Let's make this modal accessible to everyone, thalaiva!
Add ARIA attributes to improve accessibility for screen readers.
-<ion-header mode="md" class="review-split-expense-modal"> +<ion-header mode="md" class="review-split-expense-modal" role="dialog" aria-labelledby="modal-title"> <ion-toolbar> <div class="review-split-expense-modal--header"> <ion-buttons> - <ion-button (click)="close()"> + <ion-button (click)="close()" aria-label="Close modal"> <mat-icon class="fy-icon-close" svgIcon="cross"></mat-icon> </ion-button> </ion-buttons> - <ion-title>Review split expenses</ion-title> + <ion-title id="modal-title">Review split expenses</ion-title> </div> </ion-toolbar> </ion-header>Likely invalid or redundant comment.
src/app/shared/components/review-split-expense/review-split-expense.component.ts
Show resolved
Hide resolved
src/app/shared/components/review-split-expense/review-split-expense.component.spec.ts
Show resolved
Hide resolved
src/app/shared/components/review-split-expense/review-split-expense.component.scss
Show resolved
Hide resolved
src/app/shared/components/review-split-expense/review-split-expense.component.html
Show resolved
Hide resolved
|
Clickup
https://app.clickup.com/t/86cxube0g
UI Preview
Summary by CodeRabbit
New Features
Style
Tests
ReviewSplitExpenseComponent
, covering component creation, handling of empty data, and modal dismissal actions.