-
-
Notifications
You must be signed in to change notification settings - Fork 258
Add BitFileInput component (#11925) #11957
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
base: develop
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces a new BitFileInput component enabling file selection via browser input or drag-and-drop, with file metadata access and validation. Includes TypeScript interop for file handling, a DTO for file metadata, UI components for rendering selections, and comprehensive demo documentation with unit tests. Changes
Sequence DiagramsequenceDiagram
participant User
participant Browser as Browser Input
participant JS as FileInput.ts
participant Interop as JS Runtime
participant Component as BitFileInput.cs
participant Callback as OnChange Callback
User->>Browser: Select file(s) or drag-drop
Browser->>JS: Trigger change event
JS->>JS: Track file(s) in _fileInputs<br/>Generate fileId & index
JS->>Interop: setup(id, dotnetRef, inputElement, append)
Interop->>Component: JS interop method called
Component->>Component: Fetch files from JS via<br/>BitFileInputSetup
Component->>Component: Validate each file:<br/>Check MaxSize & AllowedExtensions
Component->>Component: Mark invalid files<br/>with error Message
Component->>Callback: Fire OnChange event<br/>with accumulated file list
Component->>Callback: Fire OnSelectComplete event
Callback->>User: Display selected files<br/>& validation messages
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
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.
Actionable comments posted: 7
Fix all issues with AI Agents 🤖
In
@src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/_BitFileInputItem.razor:
- Around line 18-20: The clickable remove container (div with class
"bit-fin-usi" in _BitFileInputItem.razor) is missing accessibility attributes
and keyboard support; update that element to include role="button",
tabindex="0", and an appropriate aria-label (e.g., "Remove file") while keeping
the inner icon aria-hidden="true", and wire an onkeydown handler that calls
FileInput.RemoveFile(Item) when Enter or Space is pressed so keyboard users can
activate the control.
In @src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.razor:
- Around line 36-40: In BitFileInput.razor remove the in-render mutation
"file.Index = index" from the @for loop and instead add a helper method (e.g.,
UpdateFileIndices) that iterates Files, sets Files[i].Index = i with a null
check, and call this helper whenever the Files collection is changed (for
example inside HandleOnChange and/or the Files setter). Ensure UpdateFileIndices
is invoked after any add/remove/update operations so rendering has no side
effects.
- Line 28: The input's boolean attribute is rendered as multiple="@Multiple",
which outputs multiple="false" when false and still enables multiple selection;
update BitFileInput to conditionally render the attribute based on the Multiple
property so the attribute is omitted when false (for example, replace
multiple="@Multiple" with a conditional expression that emits
multiple="multiple" only when Multiple is true, e.g., multiple="@(Multiple ?
"multiple" : null)" or wrap the input markup in an @if (Multiple) block to add
the attribute only when needed).
In @src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.ts:
- Around line 90-100: BitFileInputItem currently stores unused properties;
remove the redundant file and index fields and constructors that set them so the
class only holds the id. Update the BitFileInputItem declaration and its
constructor to accept and store just id, and ensure any code interacting with
_fileInputs (e.g., the filtering logic that uses id and the setup() method
returning files) continues to work unchanged—only the stored fields should be
removed. Leave references to BitFileInputItem, _fileInputs, and setup() intact.
In
@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Inputs/FileInput/BitFileInputDemo.razor:
- Line 213: In BitFileInputDemo.razor update the Templates example text that
reads "Supported file types: jpg, jpeg, png, bpm" to use the correct extension
"bmp" (so it matches the AllowedExtensions used elsewhere); locate the display
string in the Templates section of the BitFileInputDemo.razor component and
replace "bpm" with "bmp".
🧹 Nitpick comments (9)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.ts (3)
5-9: UnuseddotnetReferenceparameter.The
dotnetReferenceparameter is declared but never used within thesetupmethod. If it's intended for future use (e.g., invoking .NET methods from JS), consider adding a TODO comment. Otherwise, remove it to avoid confusion.
16-16: Potential null dereference oninputElement.files.The non-null assertion
inputElement.files!could throw iffilesis null (possible on some older browsers or edge cases). Consider adding a guard.🔎 Proposed defensive check
+ const inputFiles = inputElement.files; + if (!inputFiles || inputFiles.length === 0) { + return []; + } - const files = Array.from(inputElement.files!).map((file, index) => ({ + const files = Array.from(inputFiles).map((file, index) => ({
45-50: Non-null assertions ondataTransferandclipboardDatamay cause runtime errors.Both
e.dataTransfer!.filesande.clipboardData!.filesuse non-null assertions. While these are typically present during drag/paste events, defensive checks would improve robustness.🔎 Proposed defensive handling
function onDrop(e: DragEvent) { e.preventDefault(); - inputElement.files = e.dataTransfer!.files; - const event = new Event('change', { bubbles: true }); - inputElement.dispatchEvent(event); + if (e.dataTransfer?.files?.length) { + inputElement.files = e.dataTransfer.files; + const event = new Event('change', { bubbles: true }); + inputElement.dispatchEvent(event); + } } function onPaste(e: ClipboardEvent) { - inputElement.files = e.clipboardData!.files; - const event = new Event('change', { bubbles: true }); - inputElement.dispatchEvent(event); + if (e.clipboardData?.files?.length) { + inputElement.files = e.clipboardData.files; + const event = new Event('change', { bubbles: true }); + inputElement.dispatchEvent(event); + } }src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Inputs/FileInput/BitFileInputDemo.razor.cs (1)
238-286: Consider reducing handler method duplication.Ten nearly identical
HandleOnChangemethods exist. For a demo, this is acceptable for clarity, but could be consolidated using a dictionary or action delegate if maintainability becomes a concern.🔎 Optional consolidation approach
private readonly Dictionary<int, List<BitFileInputInfo>> _selectedFiles = new(); private void HandleOnChange(int exampleId, BitFileInputInfo[] files) { _selectedFiles[exampleId] = files.ToList(); } // Usage in razor: OnChange="@(files => HandleOnChange(1, files))"src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Inputs/FileInput/BitFileInputDemo.razor.scss (1)
37-54: Consider responsiveness for fixed dimensions.The
.browse-fileclass uses fixedwidth: 420pxandheight: 200px. On smaller screens, this may cause horizontal overflow. Consider usingmax-widthwith percentage-based fallback for better responsiveness.🔎 Suggested responsive adjustment
.browse-file { border: 1px solid #D2D2D7; border-radius: 2px; padding: 24px; - width: 420px; + width: 100%; + max-width: 420px; height: 200px;src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/_BitFileInputItem.razor.cs (1)
7-8: Consider adding[EditorRequired]for required parameters.Both
FileInputandItemappear to be required for the component to function. Adding[EditorRequired]would provide compile-time warnings when these parameters are not set.🔎 Proposed improvement
- [Parameter] public BitFileInput FileInput { get; set; } = default!; - [Parameter] public BitFileInputInfo Item { get; set; } = default!; + [Parameter, EditorRequired] public BitFileInput FileInput { get; set; } = default!; + [Parameter, EditorRequired] public BitFileInputInfo Item { get; set; } = default!;src/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/FileInput/BitFileInputTests.cs (2)
9-23: Test doesn't verify different behavior for enabled vs disabled states.The test uses
DataRow(true)andDataRow(false)forisEnabled, but only asserts that the element exists in both cases. Consider asserting distinct behavior, such as the presence or absence of a disabled-related CSS class on the root element.🔎 Suggested improvement
public void BitFileInputHasBasicClass(bool isEnabled) { var com = RenderComponent<BitFileInput>(parameters => { parameters.Add(p => p.IsEnabled, isEnabled); }); var bitFileInput = com.Find(".bit-fin-fi"); Assert.IsNotNull(bitFileInput); + var rootElement = com.Find(".bit-fin"); + Assert.AreEqual(!isEnabled, rootElement.ClassList.Contains("bit-dis")); }
77-86: Test does not verify remove button functionality.This test only checks that the component renders without error. It should assert that a remove button element is actually rendered when
ShowRemoveButton = true. However, since the remove button likely only appears when files are selected, this may require mocking file selection state.🔎 Suggested improvement
[TestMethod] public void BitFileInputShowRemoveButtonTest() { var com = RenderComponent<BitFileInput>(parameters => { parameters.Add(p => p.ShowRemoveButton, true); }); - Assert.IsNotNull(com); + // Verify component renders successfully with ShowRemoveButton enabled + Assert.IsNotNull(com); + // TODO: Add test with mocked file selection to verify remove button appears + // when files are selected and ShowRemoveButton is true }src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.razor.cs (1)
216-217: Clarify the distinction betweenOnChangeandOnSelectComplete.Both callbacks are invoked with identical data at the same point in execution. Based on the parameter documentation (lines 74-82),
OnChangeis for when selection changes andOnSelectCompleteis for when files are selected and validated. Consider whetherOnChangeshould fire before validation or include only valid files, to provide a clearer semantic distinction.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (15)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInfo.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.scsssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInputJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/_BitFileInputItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/_BitFileInputItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Styles/components.scsssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Inputs/FileInput/BitFileInputDemo.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Inputs/FileInput/BitFileInputDemo.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Inputs/FileInput/BitFileInputDemo.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.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/FileInput/BitFileInputTests.cs
🧰 Additional context used
🧬 Code graph analysis (4)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/_BitFileInputItem.razor.cs (2)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.ts (1)
FileInput(2-88)src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInfo.cs (1)
BitFileInputInfo(5-41)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.ts (1)
src/BlazorUI/Bit.BlazorUI/Scripts/Utils.ts (1)
Utils(2-125)
src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Inputs/FileInput/BitFileInputDemo.razor.cs (1)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.razor.cs (6)
BitFileInput(6-255)Task(111-121)Task(126-130)Task(158-163)Task(165-172)Task(185-218)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.razor.cs (3)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInfo.cs (1)
BitFileInputInfo(5-41)src/BlazorUI/Bit.BlazorUI/Extensions/StringExtensions.cs (1)
HasNoValue(12-17)src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInputJsRuntimeExtensions.cs (4)
ValueTask(17-22)ValueTask(24-27)ValueTask(29-32)ValueTask(34-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build Bit.BlazorUI
🔇 Additional comments (9)
src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Inputs/FileInput/BitFileInputDemo.razor.cs (1)
1-639: LGTM - Comprehensive demo coverage.The demo page provides thorough coverage of BitFileInput features including basic usage, multiple selection, auto-reset, append mode, max size validation, allowed extensions, remove button, events, custom templates, and public API access. The example code strings are well-documented.
src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Shared/MainLayout.razor.NavItems.cs (1)
31-31: LGTM!The FileInput navigation item follows the established pattern, is correctly placed in alphabetical order within the Inputs section, and includes the appropriate
AdditionalUrlsfor the hyphenated URL variant.src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Home/ComponentsSection.razor (1)
41-43: LGTM!The FileInput link is correctly added to the Inputs section, follows the established BitLink pattern, and is properly placed in alphabetical order.
src/BlazorUI/Bit.BlazorUI/Styles/components.scss (1)
15-15: LGTM!The BitFileInput.scss import is correctly added following the established pattern and alphabetical ordering.
src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Inputs/FileInput/BitFileInputDemo.razor.scss (1)
1-153: LGTM - Well-structured demo styles.The SCSS is well-organized with logical class naming and appropriate nesting. The styles effectively support the demo UI with clear visual hierarchy and interactive states.
src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/_BitFileInputItem.razor.cs (1)
1-12: LGTM - Clean, focused component.The component is well-structured with appropriate class naming that follows the
bit-*convention. TheGetFileElClasshelper provides clear logic for validation-based styling.src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInputJsRuntimeExtensions.cs (1)
1-38: LGTM!The JS interop extension methods are well-structured. The
[DynamicDependency]attribute on line 7 correctly preservesBitFileInputInfofor AOT/trimming scenarios, ensuring JSON deserialization works in trimmed apps. The extension method pattern provides a clean API for component-level JS interop calls.src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.razor.cs (2)
222-254: LGTM on disposal pattern.The disposal logic correctly handles JS interop exceptions that can occur during Blazor Server circuit disconnection or MAUI app refresh scenarios. The null checks before disposal and the
JSDisconnectedExceptionhandling are appropriate.
6-15: LGTM on component structure.The component is well-organized with clear separation between private state, injected dependencies, parameters, and public API. The use of
default!for_dropZoneRefand_dotnetObjis acceptable since they're initialized inOnAfterRenderAsyncbefore use, with null checks inDisposeAsync.
closes #11925
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.