-
-
Notifications
You must be signed in to change notification settings - Fork 322
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
refactor(Modal): use BootstrapBlazorRootOutlet wrap Modal component #5595
Conversation
This reverts commit 096dc3f.
Reviewer's Guide by SourceryThis pull request refactors the Modal component to be wrapped by BootstrapBlazorRootOutlet. This change ensures that the Modal component is correctly integrated within the BootstrapBlazor framework, resolving issues related to rendering and styling. The changes include modifications to the Modal's Razor markup, associated CSS/SCSS files, and updates to unit tests to reflect the new structure. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @ArgoZhang - I've reviewed your changes - here's some feedback:
Overall Comments:
- The changes to ModalTest.cs look good, but the tests are now creating a BootstrapBlazorRoot component, but not disposing of it.
- Consider adding a CSS class to the Modal component instead of relying on AdditionalAttributes for styling.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5595 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 649 649
Lines 29629 29629
Branches 4169 4165 -4
=========================================
Hits 29629 29629 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Link issues
fixes #5594
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Refactors the Modal component to use BootstrapBlazorRootOutlet to wrap the Modal component, ensuring proper rendering and functionality within the BootstrapBlazor framework. This change addresses issue #5594.
Enhancements: