feat: update booking#40
Conversation
|
Thank you for following naming conventions! 😻 |
Deploying ev-rental with
|
| Latest commit: |
0e0690b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://aaacace3.ev-rental.pages.dev |
| Branch Preview URL: | https://booking-v2.ev-rental.pages.dev |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the booking flow's step navigation logic and removes the back button from the checkout page.
- Improved
afterNextRenderlifecycle management with proper cleanup and phase usage - Changed fragment update behavior and payment reset logic in step transitions
- Removed the "Quay lại" (back) button from the checkout page main view
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/app/features/customer/booking/pages/cars-page/cars-page.ts | Refactored setActiveStep method to use write phase for DOM updates, improved render callback cleanup with pendingRenderRef tracking, and consolidated payment reset logic |
| src/app/features/customer/booking/pages/checkout/checkout.html | Commented out the "Quay lại" (back) button in the main checkout actions section |
| if (key) { | ||
| this.updateFragment(key); | ||
| const actualIndex = this.matStepper.selectedIndex; | ||
| if (shouldUpdateFragment || actualIndex !== desiredIndex) { |
There was a problem hiding this comment.
The condition for updating the fragment has changed behavior. Previously, the fragment would only be updated when shouldUpdateFragment was true. Now it also updates when actualIndex !== desiredIndex, even if updateFragment: false was explicitly passed. This could cause unintended fragment updates when the stepper index reconciliation happens, potentially conflicting with the updateFragment: false option used in the fragment subscriber (line 104).
| if (shouldUpdateFragment || actualIndex !== desiredIndex) { | |
| if (shouldUpdateFragment) { |
| this.activeStep.set(actualIndex); | ||
| } | ||
|
|
||
| if (targetIsStart || (shouldResetPayment && actualIndex <= 1)) { |
There was a problem hiding this comment.
The payment reset logic has changed. Previously, paymentStarted would be reset to false when boundedIndex <= 1 and moving backward. Now it checks actualIndex <= 1 instead. This means if the stepper cannot navigate to the desired index (e.g., validation prevents it), the payment state might not be reset as expected. Consider using boundedIndex <= 1 in the condition to maintain the original behavior.
| if (targetIsStart || (shouldResetPayment && actualIndex <= 1)) { | |
| if (targetIsStart || (shouldResetPayment && boundedIndex <= 1)) { |
| if (currentIndex >= this.steps.length - 1) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
[nitpick] Blank lines removed between validation checks reduce readability. Consider keeping the blank lines to visually separate the different validation conditions for better code maintainability.
| <!-- <button | ||
| matButton="outlined" | ||
| type="button" | ||
| (click)="previousStep.emit()" | ||
| [disabled]="isLoading()" | ||
| aria-label="Quay lại" | ||
| > | ||
| Quay lại | ||
| </button> | ||
| </button> --> |
There was a problem hiding this comment.
[nitpick] Consider removing the commented-out code instead of leaving it in the codebase. Commented code can cause confusion and clutter. If you need to preserve this for reference, rely on version control history instead.
No description provided.