chore(modal): remove rxjs dependency
#105
Conversation
use `@angular/core/rxjs-interop` instead
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request modifies the Changes
Sequence DiagramsequenceDiagram
participant Modal as ModalComponent
participant Router as RouterService
Modal->>Router: Subscribe to NavigationStart events
activate Router
Router-->>Modal: NavigationStart event
alt Modal is Open
Modal->>Modal: Close Modal
end
deactivate Router
Possibly related PRs
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/flowbite-angular/modal/modal.component.ts (1)
156-167: Consider improving code organization and documentationWhile the implementation is functionally correct, consider these improvements for better maintainability:
- The comment could be more descriptive about the purpose of closing modals during navigation
- The subscription logic could be moved to a separate private method for better organization
Here's a suggested refactor:
//#region constructor constructor() { super(); - - // close modal if it's not destroyed on route change - this.router.events.pipe(takeUntilDestroyed()).subscribe((event) => { - if (this.isOpen() && event instanceof NavigationStart) { - this.close(); - } - }); + this.initializeNavigationHandler(); } //#endregion +private initializeNavigationHandler(): void { + // Close modal during navigation to prevent it from persisting on the new route + // This ensures clean navigation and prevents modal state from bleeding across routes + this.router.events.pipe(takeUntilDestroyed()).subscribe((event) => { + if (this.isOpen() && event instanceof NavigationStart) { + this.close(); + } + }); +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/flowbite-angular/modal/modal.component.ts(4 hunks)
🔇 Additional comments (3)
libs/flowbite-angular/modal/modal.component.ts (3)
24-24: Good choice using takeUntilDestroyed!
Using takeUntilDestroyed from @angular/core/rxjs-interop is a modern approach that simplifies subscription management. This is a cleaner alternative to manual subscription cleanup with Subjects.
98-98: Clean class declaration without OnDestroy
The removal of the OnDestroy interface is appropriate since subscription cleanup is now handled by takeUntilDestroyed. This simplifies the component's lifecycle management while maintaining proper inheritance.
Line range hint 1-224: Verify modal behavior in different navigation scenarios
The implementation successfully removes the rxjs dependency while maintaining the modal's functionality. However, it's important to verify the modal behavior in various navigation scenarios.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Issue Number
Issue Number: saw this in backlog: Flowbite Angular Library
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Bug Fixes
Refactor