fix(modal): close on route change #33

[bence444]

Instead of directly manipulating the DOM, creating a template the Angular way fixes the issue.

https://angular.dev/guide/components/dom-apis

Avoid direct DOM manipulation whenever possible. Always prefer expressing your DOM's structure in component templates and updating that DOM with bindings.
Render callbacks never run during server-side rendering or build-time pre-rendering.

In this way, if the component that contains the modal gets destroyed, it automatically clears up the template (including the backdrop).

I also added a listener for router changes, so the modal doesn't stay open if it was opened outside of a router outlet

Summary by CodeRabbit

• New Features

  • Enhanced modal functionality with improved lifecycle management and dynamic template rendering.
  • Introduced a new modalWrapperClass for better styling options.

• Bug Fixes

  • Improved responsiveness of the modal to routing events, ensuring it closes appropriately during navigation.

• Documentation

  • Updated modal theme structure for better customization and flexibility in styling.

[vercel]

@bence444 is attempting to deploy a commit to the Themesberg Team on Vercel.

A member of the Team first needs to authorize it.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The changes primarily focus on the ModalComponent, ModalThemeService, and ModalTheme interface within the modal functionality of the Angular library. The ModalComponent is now standalone and implements the OnDestroy lifecycle interface, enhancing lifecycle management. The service and theme files have been updated to streamline class generation and introduce new properties for improved styling. These modifications include the addition of a modalWrapperClass and adjustments to existing properties, promoting better customization and responsiveness to routing events.

Changes

File Path	Change Summary
libs/flowbite-angular/src/lib/components/modal/modal.component.ts	- Updated ModalComponent to be standalone and implement OnDestroy. - Added destroyed property and ngOnDestroy method. - Refactored changeBackdrop method and introduced createTemplate and destroyTemplate methods. - Modified contentClasses method to include modalWrapperClass.
libs/flowbite-angular/src/lib/components/modal/modal.theme.service.ts	- Simplified getClasses method by modifying rootClass construction. - Added modalWrapperClass to the output object.
libs/flowbite-angular/src/lib/components/modal/modal.theme.ts	- Updated ModalTheme interface to include wrapper property and remove isOpen from root. - Modified modalTheme constant to reflect these changes. - Extended ModalClass interface to include modalWrapperClass.

Poem

🐇 In a modal bright and new,
With wrappers and classes, oh what a view!
Events now close it with grace,
A standalone charm in its place.
Hopping through code, we cheer and play,
For a better modal, hip-hip-hooray! 🎉

---

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?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[MGREMY]

You will need to update the theme service associated with this component.
You will have to update the ModalClass interface, because the rootClass is applied to the host component directly from BaseComponent.

Otherwise, I think that's a really good job !

[MGREMY]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)

libs/flowbite-angular/src/lib/components/modal/modal.theme.ts (1)

37-41: Approve changes to ModalTheme interface with a suggestion.

The addition of the wrapper property enhances the flexibility of modal styling and positioning. Removing isOpen from root simplifies the interface, which is a good practice.

Consider adding a comment explaining the purpose of the wrapper property for better code documentation.

You could add a comment like this:

wrapper: {
  base: string;
  position: ModalPositions;
  // Controls the styling and positioning of the modal's outer wrapper
},

libs/flowbite-angular/src/lib/components/modal/modal.component.ts (1)

175-180: Check if the modal is open before closing on Escape key press.

In the onKeydownHandler method, consider verifying if the modal is open before invoking this.close(). This ensures that the close operation is only executed when necessary.

Modify the method as follows:

onKeydownHandler(event: KeyboardEvent) {
-  if (event.key === 'Escape') {
+  if (event.key === 'Escape' && this.isOpen()) {
     this.close();
   }
}

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 1c701a3 and 26cb099.

📒 Files selected for processing (3)

• libs/flowbite-angular/src/lib/components/modal/modal.component.ts (6 hunks)
• libs/flowbite-angular/src/lib/components/modal/modal.theme.service.ts (1 hunks)
• libs/flowbite-angular/src/lib/components/modal/modal.theme.ts (3 hunks)

🧰 Additional context used

🔇 Additional comments (7)

libs/flowbite-angular/src/lib/components/modal/modal.theme.service.ts (1)

17-18: Review of changes in getClasses method

The changes to the getClasses method introduce both improvements and potential concerns:

1. Simplification of rootClass:
   The rootClass property now only uses theme.root.base, removing the dynamic class application based on the modal's open state and position. While this simplifies the code, it might reduce flexibility in styling.

2. Addition of modalWrapperClass:
   The new modalWrapperClass property combines theme.wrapper.base and position-specific classes, providing better control over the modal's wrapper element. This addition aligns well with the PR objective of improving modal behavior.

Could you please clarify the reasoning behind simplifying the rootClass? Are the open state and position styles now handled elsewhere, or is this intentional removal of functionality?

To verify the impact of these changes, let's check for any usage of the removed properties:

This will help us understand if these properties are still needed or if their removal is part of a larger refactoring effort.

✅ Verification successful

Verification Successful

The rootClass simplification is safe as the isOpen and position properties are not used elsewhere in the modal components.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usage of isOpen and position properties in modal-related files
rg --type typescript 'root\.isOpen|root\.position' libs/flowbite-angular/src/lib/components/modal/

Length of output: 135

---

Script:

#!/bin/bash
# Search for usage of isOpen and position properties in TypeScript modal-related files
rg --glob '*.ts*' 'root\.isOpen|root\.position' libs/flowbite-angular/src/lib/components/modal/

Length of output: 97

libs/flowbite-angular/src/lib/components/modal/modal.theme.ts (3)

84-84: Approve addition of modalWrapperClass to ModalClass interface.

The addition of modalWrapperClass to the ModalClass interface is a good enhancement. It aligns well with the changes made to the ModalTheme interface and provides more flexibility for custom styling of the modal wrapper.

---

Line range hint 1-86: Overall assessment of changes to modal.theme.ts

The modifications to this file significantly enhance the flexibility and customization options for the modal component. The introduction of the wrapper property in the ModalTheme interface, along with the corresponding changes in the modalTheme constant and the addition of modalWrapperClass to the ModalClass interface, provide a more robust framework for controlling modal positioning and styling.

These changes align well with the PR objectives of improving modal behavior and avoiding direct DOM manipulation. They support better integration with Angular's templating system by providing more declarative ways to control the modal's appearance and behavior.

The only point that might need further clarification is the decision to set root.base to an empty string in the modalTheme constant. Once this is addressed, these changes appear to be a solid improvement to the modal theming system.

---

53-56: Approve modalTheme changes with a request for clarification.

The addition of the wrapper object with comprehensive CSS classes enhances the modal's default styling and responsiveness. This is a positive change that provides better out-of-the-box styling.

However, I noticed that root.base is now set to an empty string.

Could you clarify the reasoning behind setting root.base to an empty string? This might remove default styling, which could be intentional but might also lead to unexpected behavior.

To verify the impact of this change, let's search for usages of root.base:

✅ Verification successful

Verified modalTheme Changes

The addition of the wrapper object with comprehensive CSS classes enhances the modal's default styling and responsiveness as intended.

Additionally, verification shows that modalTheme.root.base is not used elsewhere in the codebase, ensuring that setting it to an empty string does not introduce any unintended side effects.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usages of modalTheme.root.base
rg --type typescript 'modalTheme\.root\.base' libs/

Length of output: 88

---

Script:

#!/bin/bash
# Search for usages of modalTheme.root.base in TypeScript files
rg 'modalTheme\.root\.base' --type-add 'typescript:*.ts' --type-add 'typescript:*.tsx' --type typescript libs/

Length of output: 112

libs/flowbite-angular/src/lib/components/modal/modal.component.ts (3)

119-124: Subscription management is properly handled.

Good use of takeUntil(this.destroyed) to ensure the router events subscription is cleaned up when the component is destroyed. This prevents potential memory leaks associated with lingering subscriptions.

---

69-70: Effective lifecycle management with OnDestroy and Subject.

Implementing the OnDestroy interface and utilizing private readonly destroyed = new Subject<void>(); ensures proper cleanup of subscriptions and aligns with Angular best practices for component lifecycle management.

---

82-85: Dynamic template rendering enhances component flexibility.

Using viewChild and ViewContainerRef to manage the modal template dynamically is an effective approach. It improves the flexibility and reusability of the component by allowing dynamic insertion and removal of the modal content.

[coderabbitai]

⚠️ Potential issue

Ensure embeddedView is reset after destroying the template.

In the destroyTemplate method, after calling this.embeddedView?.destroy();, it's advisable to set this.embeddedView to undefined. This prevents potential issues with dangling references or attempting to destroy the view multiple times.

Apply this diff to reset embeddedView after destruction:

private destroyTemplate() {
  this.embeddedView?.destroy();
+  this.embeddedView = undefined;
}

📝 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.

Suggested change

	private destroyTemplate() {
	this.embeddedView?.destroy();
	}
	private destroyTemplate() {
	this.embeddedView?.destroy();
	this.embeddedView = undefined;
	}