-
-
Notifications
You must be signed in to change notification settings - Fork 331
fix(Modal): add show style in multiple dialog mode #5617
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
Conversation
Reviewer's Guide by SourceryThis pull request addresses issue #5616 by improving the modal component. The changes include formatting improvements in Sequence diagram for Modal initializationsequenceDiagram
participant JSInterop as JSInterop
participant Modal as Modal Component
Modal->>JSInterop: init(id, invoke, shownCallback, closeCallback)
activate JSInterop
JSInterop->>Modal: modal = new bootstrap.Modal(el, options)
JSInterop->>Modal: modal._config.backdrop = 'static' (if multiple)
JSInterop->>Modal: modal.handlerKeyboardAndBackdrop()
JSInterop->>Modal: el.classList.add('show')
deactivate JSInterop
File-Level Changes
Assessment against 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:
- It might be better to use
classList.replace('hide', 'show')
instead ofclassList.add('show')
to avoid potential issues if the class is already present.
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 #5617 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 649 649
Lines 29622 29622
Branches 4165 4165
=========================================
Hits 29622 29622 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Link issues
fixes #5616
Summary By Copilot
This pull request includes changes to the
Modal
component in theBootstrapBlazor
library. The modifications aim to improve the structure and functionality of the modal dialog.Improvements to modal structure:
src/BootstrapBlazor/Components/Modal/Modal.razor
: Reformatted thediv
element attributes to improve readability by placingdata-bs-backdrop
anddata-bs-keyboard
on separate lines.Enhancements to modal functionality:
src/BootstrapBlazor/Components/Modal/Modal.razor.js
: Added a classshow
to the modal element when initializing to ensure it is displayed correctly.Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Bug Fixes: