Add ProModal component (#9503)#12420
Open
msynk wants to merge 5 commits into
Open
Conversation
WalkthroughThis PR introduces a new ChangesBitProModal Component
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.razor.cs`:
- Around line 23-26: The XML doc for the Blocking parameter is incorrect: it
currently says “Whether the Modal can be light dismissed…”, but Blocking
actually prevents light-dismiss when true (overlay click handler returns early
when ModalParameters.Blocking is true). Update the summary for the Blocking
property in BitProModal (and the duplicated summaries in BitProModalDemo and any
BitModal references) to state that when true the modal cannot be light-dismissed
by clicking the overlay; when false overlay clicks allow dismissal. Ensure the
wording clearly indicates the boolean semantics (true = prevents light-dismiss,
false = allows it).
In `@src/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.scss`:
- Line 1: In BitProModal.scss update the `@import` to omit the .scss extension
(change the import of 'functions.scss' to 'functions') to satisfy the stylelint
rule scss/load-partial-extension; locate the `@import` line in BitProModal.scss
that references '../../../Bit.BlazorUI/Styles/functions.scss' and remove the
'.scss' suffix (or alternatively add/configure a stylelint suppression/baseline
for this rule if you intend to keep extensions across Extras files).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 839c5e64-71a6-48d0-8212-86f5b4be13b0
📒 Files selected for processing (10)
src/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.scsssrc/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModalClassStyles.cssrc/BlazorUI/Bit.BlazorUI.Extras/Styles/extra-components.scsssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Extras/ProModal/BitProModalDemo.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Extras/ProModal/BitProModalDemo.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Extras/ProModal/BitProModalDemo.razor.scsssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Home/ComponentsSection.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Shared/MainLayout.razor.NavItems.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
msynk
requested review from
Cyrus-Sushiant,
mhrastegari,
yasmoradi and
zahra-yousefi
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Dialog/BitDialog.ts (1)
59-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
removeDragDropcan leak listener state when the drag element is already unmounted.When
querySelectorreturns null, the function returns before removing document listeners and deleting the registry entry.Suggested patch
public static removeDragDrop(id: string, dragElementSelector: string) { const listeners = Dialog._dragDropListeners[id]; if (!listeners) return; const dragElement = document.querySelector(dragElementSelector)! as HTMLElement; - if (!dragElement) return; - - dragElement.removeEventListener('pointerdown', listeners['pointerdown']); - dragElement.style.cursor = ''; - dragElement.classList.remove('bit-mdl-nta'); + if (dragElement) { + dragElement.removeEventListener('pointerdown', listeners['pointerdown']); + dragElement.style.cursor = ''; + dragElement.classList.remove('bit-mdl-nta'); + } document.removeEventListener('pointermove', listeners['pointermove']); document.removeEventListener('pointerup', listeners['pointerup']); delete listeners['pointerdown']; delete listeners['pointermove']; delete listeners['pointerup']; delete Dialog._dragDropListeners[id]; }Also applies to: 70-80
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Dialog/BitDialog.ts` around lines 59 - 65, removeDragDrop currently exits early if document.querySelector(dragElementSelector) returns null, leaking the entry in Dialog._dragDropListeners and leaving document listeners attached; instead, always read Dialog._dragDropListeners[id] and, even when dragElement is null, remove the stored document-level listeners (using the listener functions from the listeners object) and then delete Dialog._dragDropListeners[id]; only skip element-specific cleanup when dragElement is null. Apply the same fix to the other branch/duplicate logic around lines 70-80 that also assumes the element exists.
🧹 Nitpick comments (4)
src/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.razor.cs (1)
246-285: 💤 Low valueConsider adding clarifying comments for
_offsetTopandAbsolutePositionlogic.The lifecycle logic is correct but subtle:
Line 259 sets
_offsetTop = 0before callingToggleScroll. This appears redundant sinceToggleScroll(lines 341, 345) overwrites_offsetTop, but it's actually a guard for whenAutoToggleScrollis false (line 337 early return).Lines 263-267 reset and re-render only for
AbsolutePosition. This ensures the top offset style (registered on line 243) is applied afterToggleScrollcalculates the scroll offset.📝 Suggested clarifying comments
- _offsetTop = 0; + // Reset offset; will be recalculated by ToggleScroll if AutoToggleScroll is enabled + _offsetTop = 0; await ToggleScroll(true); if (AbsolutePosition) { + // Re-render to apply the calculated top offset style for absolute positioning StyleBuilder.Reset(); StateHasChanged(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.razor.cs` around lines 246 - 285, Add brief clarifying comments inside OnAfterRenderAsync around the _offsetTop and AbsolutePosition logic: comment why _offsetTop = 0 is set before calling ToggleScroll (it's a guard for the case when AutoToggleScroll is false and ToggleScroll will not recalc _offsetTop), and comment why StyleBuilder.Reset() and StateHasChanged() are invoked only when AbsolutePosition is true (to ensure the computed top-offset style, which ToggleScroll may have updated, is applied after render). Reference the symbols _offsetTop, ToggleScroll, AutoToggleScroll, AbsolutePosition, StyleBuilder.Reset, and OnAfterRenderAsync when placing the comments.src/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModalClassStyles.cs (1)
3-34: ⚡ Quick winConsider adding a
Mergemethod forBitProModalClassStyles.The base
BitModalClassStylesprovides a staticMergemethod, but it only merges the base properties (Root,Overlay,Content). WhenBitProModalClassStylesinstances need to be merged, the derived properties (HeaderContainer,Header,CloseButton,CloseIcon,Body,Footer) will be lost.📦 Proposed Merge method
+ public static BitProModalClassStyles Merge(BitProModalClassStyles? classStyles1, BitProModalClassStyles? classStyles2) + { + var baseStyles = BitModalClassStyles.Merge(classStyles1, classStyles2); + return new BitProModalClassStyles + { + Root = baseStyles.Root, + Overlay = baseStyles.Overlay, + Content = baseStyles.Content, + HeaderContainer = classStyles1?.HeaderContainer ?? classStyles2?.HeaderContainer, + Header = classStyles1?.Header ?? classStyles2?.Header, + CloseButton = classStyles1?.CloseButton ?? classStyles2?.CloseButton, + CloseIcon = classStyles1?.CloseIcon ?? classStyles2?.CloseIcon, + Body = classStyles1?.Body ?? classStyles2?.Body, + Footer = classStyles1?.Footer ?? classStyles2?.Footer + }; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModalClassStyles.cs` around lines 3 - 34, The BitProModalClassStyles class lacks a Merge method so derived properties (HeaderContainer, Header, CloseButton, CloseIcon, Body, Footer) are not preserved when merging; add a static Merge(BitProModalClassStyles? first, BitProModalClassStyles? second) that calls BitModalClassStyles.Merge for base fields then merges each derived string property (choose non-null/longer/concatenate as project convention) into the result, ensuring the returned BitProModalClassStyles contains merged values for Root/Overlay/Content plus HeaderContainer, Header, CloseButton, CloseIcon, Body, and Footer.src/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.scss (1)
37-110: ⚖️ Poor tradeoffSignificant duplication in positioning classes.
Six pairs of positioning classes are identical, resulting in 36+ lines of duplicate CSS:
.bit-mdl-tlf(TopLeft) =.bit-mdl-tst(TopStart).bit-mdl-trg(TopRight) =.bit-mdl-ten(TopEnd).bit-mdl-clf(CenterLeft) =.bit-mdl-cst(CenterStart).bit-mdl-crg(CenterRight) =.bit-mdl-cen(CenterEnd).bit-mdl-blf(BottomLeft) =.bit-mdl-bst(BottomStart).bit-mdl-brg(BottomRight) =.bit-mdl-ben(BottomEnd)Additionally, this causes incorrect RTL behavior: absolute directions (Left/Right) currently use flexbox
flex-start/flex-end, which adapt to thedirattribute. In RTL mode,TopLeftwill incorrectly position on the right. Absolute directions should use fixed positioning (left: 0,right: 0) while logical directions (Start/End) should continue using flexbox.🎨 Refactor approach
For absolute directions, replace flexbox with explicit positioning:
.bit-mdl-tlf { top: 0; left: 0; } .bit-mdl-trg { top: 0; right: 0; }Keep logical directions using flexbox (they correctly adapt to
dir):.bit-mdl-tst { align-items: flex-start; justify-content: flex-start; }This eliminates duplication and fixes RTL behavior for absolute directions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.scss` around lines 37 - 110, Duplicate positioning classes cause redundant rules and break RTL: replace the absolute-direction classes (.bit-mdl-tlf, .bit-mdl-trg, .bit-mdl-clf, .bit-mdl-crg, .bit-mdl-blf, .bit-mdl-brg) so they use explicit positioning (top/left/top/right/bottom/left/bottom/right) instead of flexbox, and keep the logical-direction classes (.bit-mdl-tst, .bit-mdl-ten, .bit-mdl-cst, .bit-mdl-cen, .bit-mdl-bst, .bit-mdl-ben) using flexbox (align-items/justify-content) to preserve RTL behavior; remove the duplicated flexbox rules from the absolute classes and consolidate identical rules to avoid repetition.src/BlazorUI/Bit.BlazorUI/Extensions/IBitBlazorUIServiceCollectionExtensions.cs (1)
10-17: Consider documenting singleton usage constraints.When
trySingleton=true,BitModalServiceis registered as a singleton with a single_containerfield. If multipleBitModalContainerinstances exist in the application, the last container to callInitContainerwill become the active container, potentially causing persistent modals queued before that point to be rendered in an unexpected location.Applications using
trySingleton=trueshould ensure only oneBitModalContainerexists, or document this behavior for developers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Extensions/IBitBlazorUIServiceCollectionExtensions.cs` around lines 10 - 17, The BitModalService singleton registration (controlled by trySingleton) uses a single _container and when multiple BitModalContainer instances call InitContainer the last caller wins; update documentation to state that if you pass trySingleton=true you must ensure only one BitModalContainer exists (or explain the last-InitContainer-wins behavior and that scoped registration avoids it), and add this note to the public API surface — e.g., XML docs on IBitBlazorUIServiceCollectionExtensions.AddBitBlazorUI/BitModalService registration method and/or the BitModalService class mentioning trySingleton, InitContainer, and the expected constraint so developers are aware.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.ts`:
- Around line 6-12: The removal logic can miss the original drag handle if the
selector changed because unbinding resolves the drag element from the current
selector instead of the one that was originally bound; update the ProModal
drag-drop registration to save the concrete elements (or at least the
dragElement reference) inside ProModal._dragDropListeners[containerSelector]
when adding, and then in ProModal.removeDragDrop use that stored dragElement and
stored listener references to remove the pointerdown handler and any visual CSS
class before deleting the entry; apply the same change to the other removal site
(the block around the second occurrence at the 52-61 region) so unbinding always
uses the originally-bound element instead of querying the selector again.
In `@src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Modal/BitModal.razor`:
- Around line 13-16: The markup reads component fields (AriaModal and
ShowOverlay) directly which bypasses overrides in ModalParameters after Merge;
update the template to use ModalParameters.AriaModal and
ModalParameters.ShowOverlay (and ensure GetRole() reads from ModalParameters or
add a GetRole(ModalParameters) overload) so aria-modal and role reflect
merged/overridden values from ModalParameters instead of the internal fields.
In `@src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Modal/BitModalService.cs`:
- Line 135: The overlay click handler is always set to close the modal which
allows blocking modals to be dismissed; update the builder.AddComponentParameter
call that sets BitModal.OnOverlayClick (currently using
EventCallback.Factory.Create<MouseEventArgs>(modalReference, () =>
modalReference.Close())) to only wire that EventCallback when
modalParameters?.Blocking is false (or not true), and pass null/omit the
parameter when modalParameters?.Blocking is true so blocking modals cannot be
closed by clicking the overlay.
In
`@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Surfaces/Modal/BitModalServiceDemo.razor`:
- Line 17: The example currently has the BitModalContainer commented out so
modals created via BitModalService will not render; either uncomment the
BitModalContainer line (restore the component invocation, e.g.,
BitModalContainer ModalParameters="@(new() { FullWidth = true })") so the demo
works, or add a brief explanatory note near the Show button/BitModalService
usage stating that consumers must include <BitModalContainer /> (or equivalent)
in their layout/root component to render modals; update references in this file
(BitModalContainer, BitModalService) accordingly so the demo is functional or
clearly documents the requirement.
---
Outside diff comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Dialog/BitDialog.ts`:
- Around line 59-65: removeDragDrop currently exits early if
document.querySelector(dragElementSelector) returns null, leaking the entry in
Dialog._dragDropListeners and leaving document listeners attached; instead,
always read Dialog._dragDropListeners[id] and, even when dragElement is null,
remove the stored document-level listeners (using the listener functions from
the listeners object) and then delete Dialog._dragDropListeners[id]; only skip
element-specific cleanup when dragElement is null. Apply the same fix to the
other branch/duplicate logic around lines 70-80 that also assumes the element
exists.
---
Nitpick comments:
In `@src/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.razor.cs`:
- Around line 246-285: Add brief clarifying comments inside OnAfterRenderAsync
around the _offsetTop and AbsolutePosition logic: comment why _offsetTop = 0 is
set before calling ToggleScroll (it's a guard for the case when AutoToggleScroll
is false and ToggleScroll will not recalc _offsetTop), and comment why
StyleBuilder.Reset() and StateHasChanged() are invoked only when
AbsolutePosition is true (to ensure the computed top-offset style, which
ToggleScroll may have updated, is applied after render). Reference the symbols
_offsetTop, ToggleScroll, AutoToggleScroll, AbsolutePosition,
StyleBuilder.Reset, and OnAfterRenderAsync when placing the comments.
In `@src/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.scss`:
- Around line 37-110: Duplicate positioning classes cause redundant rules and
break RTL: replace the absolute-direction classes (.bit-mdl-tlf, .bit-mdl-trg,
.bit-mdl-clf, .bit-mdl-crg, .bit-mdl-blf, .bit-mdl-brg) so they use explicit
positioning (top/left/top/right/bottom/left/bottom/right) instead of flexbox,
and keep the logical-direction classes (.bit-mdl-tst, .bit-mdl-ten,
.bit-mdl-cst, .bit-mdl-cen, .bit-mdl-bst, .bit-mdl-ben) using flexbox
(align-items/justify-content) to preserve RTL behavior; remove the duplicated
flexbox rules from the absolute classes and consolidate identical rules to avoid
repetition.
In
`@src/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModalClassStyles.cs`:
- Around line 3-34: The BitProModalClassStyles class lacks a Merge method so
derived properties (HeaderContainer, Header, CloseButton, CloseIcon, Body,
Footer) are not preserved when merging; add a static
Merge(BitProModalClassStyles? first, BitProModalClassStyles? second) that calls
BitModalClassStyles.Merge for base fields then merges each derived string
property (choose non-null/longer/concatenate as project convention) into the
result, ensuring the returned BitProModalClassStyles contains merged values for
Root/Overlay/Content plus HeaderContainer, Header, CloseButton, CloseIcon, Body,
and Footer.
In
`@src/BlazorUI/Bit.BlazorUI/Extensions/IBitBlazorUIServiceCollectionExtensions.cs`:
- Around line 10-17: The BitModalService singleton registration (controlled by
trySingleton) uses a single _container and when multiple BitModalContainer
instances call InitContainer the last caller wins; update documentation to state
that if you pass trySingleton=true you must ensure only one BitModalContainer
exists (or explain the last-InitContainer-wins behavior and that scoped
registration avoids it), and add this note to the public API surface — e.g., XML
docs on IBitBlazorUIServiceCollectionExtensions.AddBitBlazorUI/BitModalService
registration method and/or the BitModalService class mentioning trySingleton,
InitContainer, and the expected constraint so developers are aware.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 73cadd8e-9ee2-4813-a90b-6bf437b7a48f
📒 Files selected for processing (39)
src/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.scsssrc/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModalClassStyles.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModalJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Extensions/IBlazorUIExtrasServiceCollectionExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Styles/extra-components.scsssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Dialog/BitDialog.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Dialog/BitDialog.tssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Dialog/BitDialogJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Modal/BitModal.razorsrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Modal/BitModal.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Modal/BitModal.scsssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Modal/BitModalContainer.razorsrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Modal/BitModalContainer.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Modal/BitModalJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Modal/BitModalParameters.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Modal/BitModalReference.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Modal/BitModalService.cssrc/BlazorUI/Bit.BlazorUI/Extensions/IBitBlazorUIServiceCollectionExtensions.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Extras/ModalService/BitModalServiceDemo.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Extras/ProModal/BitProModalDemo.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Extras/ProModal/BitProModalDemo.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Extras/ProModal/BitProModalDemo.razor.scsssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Surfaces/Modal/BitModalDemo.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Surfaces/Modal/BitModalDemo.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Surfaces/Modal/BitModalServiceDemo.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Surfaces/Modal/BitModalServiceDemo.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Surfaces/Modal/BitModalServiceDemo.razor.scsssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Surfaces/Modal/ModalContent.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Home/ComponentsSection.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Shared/MainLayout.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Shared/MainLayout.razor.NavItems.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/compilerconfig.jsonsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/ProModal/BitProModalTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Surfaces/Modal/BitModalServiceTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Surfaces/Modal/BitModalTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Surfaces/Modal/TestModalContent.cs
💤 Files with no reviewable changes (4)
- src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Extras/ModalService/BitModalServiceDemo.razor
- src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Modal/BitModalJsRuntimeExtensions.cs
- src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Modal/BitModalParameters.cs
- src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Modal/BitModal.scss
Comment on lines
+6
to
+12
| ProModal.removeDragDrop(containerSelector, dragElementSelector); | ||
| const listeners: any = {}; | ||
| ProModal._dragDropListeners[containerSelector] = listeners; | ||
|
|
||
| const element = document.querySelector(containerSelector) as HTMLElement; | ||
| const dragElement = document.querySelector(dragElementSelector) as HTMLElement; | ||
| if (!element || !dragElement) return; |
There was a problem hiding this comment.
Cleanup can miss the originally bound drag handle when selector changes.
Listener state is keyed only by containerSelector, but unbinding resolves the drag element from the current selector argument. If the selector changes, the old element keeps pointerdown + visual state.
Suggested patch
public static setupDragDrop(containerSelector: string, dragElementSelector: string) {
ProModal.removeDragDrop(containerSelector, dragElementSelector);
const listeners: any = {};
ProModal._dragDropListeners[containerSelector] = listeners;
const element = document.querySelector(containerSelector) as HTMLElement;
const dragElement = document.querySelector(dragElementSelector) as HTMLElement;
if (!element || !dragElement) return;
+ listeners['dragElement'] = dragElement;
@@
public static removeDragDrop(containerSelector: string, dragElementSelector: string) {
const listeners = ProModal._dragDropListeners[containerSelector];
if (!listeners) return;
- const dragElement = document.querySelector(dragElementSelector) as HTMLElement;
+ const dragElement = (listeners['dragElement'] as HTMLElement)
+ ?? (document.querySelector(dragElementSelector) as HTMLElement);
if (dragElement) {
dragElement.removeEventListener('pointerdown', listeners['pointerdown']);
dragElement.style.cursor = '';
dragElement.classList.remove('bit-mdl-nta');
}
@@
delete listeners['pointerup'];
+ delete listeners['dragElement'];
delete ProModal._dragDropListeners[containerSelector];
}Also applies to: 52-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.ts` around
lines 6 - 12, The removal logic can miss the original drag handle if the
selector changed because unbinding resolves the drag element from the current
selector instead of the one that was originally bound; update the ProModal
drag-drop registration to save the concrete elements (or at least the
dragElement reference) inside ProModal._dragDropListeners[containerSelector]
when adding, and then in ProModal.removeDragDrop use that stored dragElement and
stored listener references to remove the pointerdown handler and any visual CSS
class before deleting the entry; apply the same change to the other removal site
(the block around the second occurrence at the 52-61 region) so unbinding always
uses the originally-bound element instead of querying the selector again.
Comment on lines
+13
to
16
| aria-modal="@AriaModal.ToString().ToLowerInvariant()" | ||
| role="@GetRole()"> | ||
| @if (ModalParameters.Modeless is false) | ||
| @if (ShowOverlay) | ||
| { |
There was a problem hiding this comment.
Overlay and aria-modal rendering bypass merged modal parameters.
This reads component fields directly while the rest of the markup reads ModalParameters. That can drop container/service overrides after BitModalParameters.Merge(...).
Suggested patch
- aria-modal="`@AriaModal.ToString`().ToLowerInvariant()"
+ aria-modal="`@ModalParameters.AriaModal.ToString`().ToLowerInvariant()"
@@
- `@if` (ShowOverlay)
+ `@if` (ModalParameters.ShowOverlay)📝 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
| aria-modal="@AriaModal.ToString().ToLowerInvariant()" | |
| role="@GetRole()"> | |
| @if (ModalParameters.Modeless is false) | |
| @if (ShowOverlay) | |
| { | |
| aria-modal="`@ModalParameters.AriaModal.ToString`().ToLowerInvariant()" | |
| role="`@GetRole`()"> | |
| `@if` (ModalParameters.ShowOverlay) | |
| { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Modal/BitModal.razor` around
lines 13 - 16, The markup reads component fields (AriaModal and ShowOverlay)
directly which bypasses overrides in ModalParameters after Merge; update the
template to use ModalParameters.AriaModal and ModalParameters.ShowOverlay (and
ensure GetRole() reads from ModalParameters or add a GetRole(ModalParameters)
overload) so aria-modal and role reflect merged/overridden values from
ModalParameters instead of the internal fields.
| { | ||
| builder.AddComponentParameter(seq++, nameof(BitModal.OnOverlayClick), EventCallback.Factory.Create<MouseEventArgs>(modalReference, () => modalReference.Close())); | ||
| } | ||
| builder.AddComponentParameter(seq++, nameof(BitModal.OnOverlayClick), EventCallback.Factory.Create<MouseEventArgs>(modalReference, () => modalReference.Close())); |
There was a problem hiding this comment.
Blocking behavior is bypassed for service-created modals.
Line 135 always wires overlay click to modalReference.Close(), so a blocking modal can still be dismissed by clicking outside. Please restore conditional wiring based on modalParameters?.Blocking.
Suggested fix
builder.OpenComponent<BitModal>(seq++);
builder.SetKey(modalReference.Id);
builder.AddComponentParameter(seq++, nameof(BitModal.IsOpen), true);
- builder.AddComponentParameter(seq++, nameof(BitModal.OnOverlayClick), EventCallback.Factory.Create<MouseEventArgs>(modalReference, () => modalReference.Close()));
+ if (modalParameters?.Blocking is not true)
+ {
+ builder.AddComponentParameter(seq++, nameof(BitModal.OnOverlayClick),
+ EventCallback.Factory.Create<MouseEventArgs>(modalReference, () => modalReference.Close()));
+ }
builder.AddComponentParameter(seq++, nameof(BitModal.ChildContent), content);
builder.CloseComponent();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Modal/BitModalService.cs` at
line 135, The overlay click handler is always set to close the modal which
allows blocking modals to be dismissed; update the builder.AddComponentParameter
call that sets BitModal.OnOverlayClick (currently using
EventCallback.Factory.Create<MouseEventArgs>(modalReference, () =>
modalReference.Close())) to only wire that EventCallback when
modalParameters?.Blocking is false (or not true), and pass null/omit the
parameter when modalParameters?.Blocking is true so blocking modals cannot be
closed by clicking the overlay.
| <div>Show a FullWidth modal using the BitModalService:</div><br/> | ||
| <BitButton OnClick="ShowModal">Show</BitButton> | ||
|
|
||
| @* <BitModalContainer ModalParameters="@(new() { FullWidth = true })" /> *@ |
There was a problem hiding this comment.
Example is non-functional without BitModalContainer.
The BitModalContainer is commented out (line 17), which means the modal shown by clicking the "Show" button will not render. The container is required to display modals created via BitModalService.
If this is intentional as a code snippet example, consider:
- Uncommenting the container to make the example functional, or
- Adding explanatory text that users must add
<BitModalContainer />to their layout
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Surfaces/Modal/BitModalServiceDemo.razor`
at line 17, The example currently has the BitModalContainer commented out so
modals created via BitModalService will not render; either uncomment the
BitModalContainer line (restore the component invocation, e.g.,
BitModalContainer ModalParameters="@(new() { FullWidth = true })") so the demo
works, or add a brief explanatory note near the Show button/BitModalService
usage stating that consumers must include <BitModalContainer /> (or equivalent)
in their layout/root component to render modals; update references in this file
(BitModalContainer, BitModalService) accordingly so the demo is functional or
clearly documents the requirement.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #9503
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Chores