feat(components): introduce Dropdown component #168
Conversation
… `IPopoverService`
…uItem components
…if `DisabledItems` is used
|
Warning Rate limit exceeded@desmondinho has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis pull request integrates a new Variants project into the solution and updates numerous components across the LumexUI framework. New types, records, and collections for managing component variants have been added. Many components, including Dropdown, Popover, and Menu, have been refactored to use service-based asynchronous interactions and a slot-based styling system. Additionally, the styling system now leverages variant configuration with Tailwind integration, and documentation pages, examples, and tests have been updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Button
participant IPopoverService
participant Popover
User->>Button: Click trigger
Button->>IPopoverService: TriggerAsync(popoverId)
IPopoverService->>Popover: Lookup using popoverId
Popover-->>IPopoverService: Return popover instance
IPopoverService->>Popover: Invoke TriggerAsync()
Popover-->>User: Display popover
sequenceDiagram
participant Component
participant TwVariant
participant VariantEngine
Component->>TwVariant: Create(VariantConfig)
TwVariant->>VariantEngine: Process base, overrides, compounds
VariantEngine-->>TwVariant: Merged style slots
TwVariant-->>Component: Return ComponentVariant
Poem
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🔭 Outside diff range comments (1)
tests/LumexUI.Tests/Components/Listbox/ListboxTests.razor (1)
4-6:⚠️ Potential issueAdd missing import for TwVariant.
The
TwVarianttype is used in the service registration but its import statement is missing.Add the following import:
@using Microsoft.Extensions.DependencyInjection @using TailwindMerge +@using LumexUI.Variants
🧹 Nitpick comments (78)
tests/LumexUI.Tests/Components/Checkbox/CheckboxGroupTests.cs (1)
18-19: Fix indentation to use spaces instead of tabs.Lines 18-19 use tabs for indentation while the rest of the file uses spaces. This inconsistency should be fixed to maintain uniform code formatting.
- Services.AddSingleton<TwVariant>(); - } + Services.AddSingleton<TwVariant>(); + }tests/LumexUI.Tests/Components/Divider/DividerTests.cs (1)
19-20: Fix indentation to maintain consistency.The service registration is correctly implemented, but the indentation uses tabs instead of spaces, which is inconsistent with the rest of the file.
Apply this diff to fix the indentation:
- Services.AddSingleton<TwVariant>(); - } + Services.AddSingleton<TwVariant>(); + }docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/CustomStyles.razor (1)
10-11: Consider making the placement configurable.While the Right placement works for this example, consider making it configurable through a parameter to enhance reusability.
+ [Parameter] + public PopoverPlacement Placement { get; set; } = PopoverPlacement.Right; - Placement="@PopoverPlacement.Right" + Placement="@Placement"tests/LumexUI.Tests/Components/Icon/IconTests.cs (1)
18-19: Fix indentation inconsistency.Lines 18-19 use tabs while the rest of the file uses spaces. Maintain consistent indentation throughout the file.
- Services.AddSingleton<TwVariant>(); - } + Services.AddSingleton<TwVariant>(); + }tests/LumexUI.Tests/Components/Switch/SwitchTests.cs (1)
19-20: Fix indentation inconsistency.The indentation uses tabs instead of spaces, which is inconsistent with the rest of the file.
Apply this diff to fix the indentation:
- Services.AddSingleton<TwVariant>(); - } + Services.AddSingleton<TwVariant>(); + }tests/LumexUI.Tests/Components/Tabs/TabsTests.razor (1)
12-20: Consider adding tests for variant-related functionality.While the
TwVariantservice is now registered, there are no specific tests covering the variant system's functionality with the tabs component. Consider adding test cases to verify:
- Variant application to tabs
- Variant inheritance
- Custom variant overrides
Would you like me to help generate test cases for the variant system integration?
docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/Offset.razor (1)
10-17: Consider enhancing accessibility with ARIA attributes.While the popover implementation is functional, consider adding ARIA attributes to improve accessibility:
-<LumexPopover Id="@_popoverId" Offset="16"> +<LumexPopover Id="@_popoverId" + Offset="16" + aria-label="Additional information" + role="dialog"> <LumexPopoverContent> - <div class="px-1 py-2"> + <div class="px-1 py-2" role="presentation"> <div class="text-small font-bold">Oh, hi there!</div> <div class="text-tiny">I am the popover content</div> </div> </LumexPopoverContent> </LumexPopover>tests/LumexUI.Tests/Components/Listbox/ListboxTests.razor (1)
11-11: Consider adding variant-specific test cases.While the
TwVariantservice has been registered, there are no explicit tests covering the variant functionality of the Listbox component. Consider adding test cases to verify:
- Variant application and inheritance
- Dynamic variant changes
- Custom variant configurations
Would you like me to help generate test cases for the variant functionality?
tests/LumexUI.Tests/Components/Link/LinkTests.cs (2)
18-19: Fix indentation to use spaces instead of tabs.The indentation uses tabs while the rest of the file uses spaces. Maintain consistency with the existing code style.
- Services.AddSingleton<TwVariant>(); - } + Services.AddSingleton<TwVariant>(); + }
13-56: Add tests for variant configuration functionality.While the existing tests cover basic functionality, there are no tests verifying the new variant configuration capabilities. Consider adding tests that:
- Verify default variant application
- Test custom variant overrides
- Validate variant inheritance and composition
Would you like me to help generate test cases for the variant configuration functionality?
src/LumexUI/Theme/Colors/SemanticColors.cs (1)
39-40: Consider surface level consistency.The change makes Surface1 use pure white background while Surface2 and Surface3 use zinc shades. This creates a more distinct hierarchy but verify if this aligns with the design system's surface level progression.
Let's analyze the surface progression:
- Surface1: White background, Zinc-900 text
- Surface2: Zinc-100 background, Zinc-800 text
- Surface3: Zinc-200 background, Zinc-700 text
The progression shows increasing zinc shade intensity for backgrounds while text color gets lighter, which maintains a logical hierarchy. However, the jump from white to Zinc-100 is more pronounced than the transition between other surface levels.
tests/LumexUI.Tests/Components/Collapse/CollapseTests.cs (3)
20-21: Fix indentation inconsistency.The service registration looks good, but the indentation uses tabs instead of spaces, which is inconsistent with the rest of the file.
Apply this diff to fix the indentation:
- Services.AddSingleton<TwVariant>(); - JSInterop.Setup<int>( "Lumex.elementReference.getScrollHeight", _ => true ); + Services.AddSingleton<TwVariant>(); + JSInterop.Setup<int>( "Lumex.elementReference.getScrollHeight", _ => true );
61-61: Address the TODO comment.The TODO comment indicates that additional test coverage is needed. Since this PR introduces variant-based styling, we should add tests to verify that the Collapse component correctly applies variant styles.
Would you like me to help generate additional test cases for:
- Variant style application
- Variant style overrides
- Default variant behavior
24-59: Consider adding variant-specific test cases.The existing test methods provide good coverage of core functionality. However, with the introduction of variant-based styling, consider adding test cases to verify:
- Default variant application
- Custom variant overrides
- Dynamic variant changes
- Variant inheritance
Would you like me to help generate these additional test cases?
tests/LumexUI.Tests/Components/Popover/PopoverTests.razor (1)
25-32: Consider extracting test data to improve maintainability.The test markup is duplicated across multiple test methods. Consider extracting common test data to reduce duplication and improve maintainability.
Example approach:
private static RenderFragment GetBasicPopover(string triggerText = "Show Popover") => builder => { builder.OpenComponent<LumexPopover>(0); builder.AddContent(1, @<text> <LumexPopoverTrigger>@triggerText</LumexPopoverTrigger> <LumexPopoverContent> <p>This is the content of the popover.</p> </LumexPopoverContent> </text>); builder.CloseComponent(); };Then use it in tests:
var cut = Render(GetBasicPopover());Also applies to: 40-47, 62-69, 116-131, 152-159
docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/Usage.razor (3)
5-8: Enhance accessibility for the popover trigger button.Consider adding ARIA attributes to improve accessibility:
<LumexButton OnClick="@TriggerAsync" - data-popoverref="@_popoverId"> + data-popoverref="@_popoverId" + aria-haspopup="true" + aria-expanded="false" + aria-controls="@_popoverId">
10-17: Enhance semantic structure of the popover content.Consider adding semantic improvements for better accessibility and user experience:
-<LumexPopover Id="@_popoverId"> +<LumexPopover Id="@_popoverId" role="dialog" aria-modal="true"> <LumexPopoverContent> - <div class="px-1 py-2"> + <div class="px-1 py-2" role="document"> - <div class="text-small font-bold">Oh, hi there!</div> - <div class="text-tiny">I am the popover content</div> + <h2 class="text-small font-bold">Oh, hi there!</h2> + <p class="text-tiny">I am the popover content</p> + <button class="mt-2" @onclick="TriggerAsync" aria-label="Close popover"> + Close + </button> </div> </LumexPopoverContent> </LumexPopover>
19-26: Consider improving the robustness of the code-behind implementation.The implementation is clean, but consider these improvements:
@code { - private string _popoverId = "popover-usage"; + private string _popoverId = $"popover-usage-{Guid.NewGuid():N}"; private Task TriggerAsync() { - return PopoverService.TriggerAsync(_popoverId); + try + { + return PopoverService.TriggerAsync(_popoverId); + } + catch (Exception ex) + { + // Log the error or handle it appropriately + return Task.CompletedTask; + } } }tests/LumexUI.Tests/Components/RadioGroup/RadioGroupTests.cs (1)
18-20: Fix indentation inconsistency.The indentation of the service registrations is inconsistent. Line 18 uses spaces while lines 19-20 use tabs.
Apply this diff to maintain consistent indentation:
- Services.AddSingleton<TwMerge>(); - Services.AddSingleton<TwVariant>(); - } + Services.AddSingleton<TwMerge>(); + Services.AddSingleton<TwVariant>(); + }src/LumexUI/Styles/Button.cs (2)
45-51: Consider adding XML documentation for size values.While the implementation is clean, adding XML documentation to describe the exact dimensions and use cases for each size would improve maintainability.
+ /// <summary> + /// Gets the size-specific styles for the button. + /// </summary> + /// <param name="size"> + /// Size.Small: 16px min-width, 32px height + /// Size.Medium: 20px min-width, 40px height + /// Size.Large: 24px min-width, 48px height + /// </param> private static ElementClass GetSizeStyles(Size size)
62-68: Consider extracting duplicated border styles.The
border-2 bg-transparentstyles are duplicated for both Outlined and Ghost variants.private static ElementClass GetVariantStyles(Variant variant) { + var transparentStyle = "bg-transparent"; + var borderedTransparentStyle = $"border-2 {transparentStyle}"; return ElementClass.Empty() - .Add("border-2 bg-transparent", when: variant is Variant.Outlined) - .Add("border-2 bg-transparent", when: variant is Variant.Ghost) - .Add("bg-transparent", when: variant is Variant.Light); + .Add(borderedTransparentStyle, when: variant is Variant.Outlined) + .Add(borderedTransparentStyle, when: variant is Variant.Ghost) + .Add(transparentStyle, when: variant is Variant.Light);src/LumexUI/Components/Popover/LumexPopoverContent.razor (1)
10-14: Verify ARIA attributes for dialog accessibility.While the dialog role is correctly set, consider adding
aria-modalandaria-labelledbyattributes to enhance accessibility.<div role="dialog" + aria-modal="true" + aria-labelledby="@($"popover-{Popover.Id}-title")" class="@Popover.Slots["Base"](Popover.Classes?.Base, Popover.Class)" tabindex="-1"tests/LumexUI.Tests/Components/RadioGroup/RadioTests.cs (2)
19-20: Fix indentation inconsistency.The indentation of the closing brace uses tabs instead of spaces. This inconsistency should be fixed to maintain uniform code formatting.
- Services.AddSingleton<TwVariant>(); - } + Services.AddSingleton<TwVariant>(); + }
22-32: Consider adding variant-related test cases.While the existing test covers the radio group constraint, consider adding test cases to verify the component's behavior with different variants, given the new TwVariant service integration.
Would you like me to help generate additional test cases for variant-related scenarios?
docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/_Radius.razor (2)
5-7: Leverage the implicit string conversion when building IDs.You can shorten the expression by removing the explicit
.ToString()call since string interpolation can handle enumeration values automatically:- var id = $"popover-radius-{radius.ToString()}"; + var id = $"popover-radius-{radius}";
9-14: Consider accessibility enhancements for the popover-triggering button.While the current setup is functional, consider adding ARIA attributes or labeling to improve accessibility (e.g.,
aria-haspopup="true",aria-controls="...").docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/Sizes.razor (1)
27-30: Consider handling potential errors fromTriggerAsync.
As it stands, returningPopoverService.TriggerAsync(id)is acceptable if the service handles its own internal errors or retries. If there’s a possibility of exceptions, consider using a try/catch block or error logging to enhance debugging.src/LumexUI/Services/Popover/PopoverService.cs (1)
26-29: Consider handling duplicate popover IDs.If another popover with the same
Idis already added,Add(id, popover)will throw an exception. Decide whether to overwrite, skip, or throw a custom exception if duplicates occur.src/LumexUI.Variants/TwVariant.cs (1)
15-100: Ensure safe handling of non-existent variant values.The multi-step merging logic is structured well, but consider gracefully handling cases where
selectedValuemight not appear invalues. Currently, the code does:if (!string.IsNullOrEmpty(selectedValue) && values[selectedValue] is SlotCollection variantSlots) { ... }If
selectedValueis not invalues, this pattern is fine as is, but double-check that it won't cause runtime issues or confusion. Otherwise, the logic is well-organized.src/LumexUI/Styles/Popover.cs (1)
16-18: Consider potential concurrency or multi-usage implications ofstatic ComponentVariant? _variant.
Storing variant configuration in a static field could lead to inconsistent styling across different usages of thePopoverclass if multiple components manipulate_variantin parallel or in different contexts. Consider using a caching dictionary keyed by relevant style parameters or storing this variant configuration in instance scope if you anticipate complex usage scenarios.src/LumexUI/Styles/Menu.cs (1)
16-606: Reduce repetition in compound variants by introducing helper functions or centralized mappings.
The extensive blocks of repeatedCompoundVariantdefinitions may hinder maintainability. Creating reusable functions or dictionaries can streamline the process of registering hover effects for each variant-color combination, making it simpler to extend and reduce the risk of errors.src/LumexUI/Components/Dropdown/LumexDropdown.razor (1)
1-8: LGTM! Well-structured dropdown implementation.The component correctly inherits from LumexPopover and properly uses CascadingValue for context sharing.
Consider adding XML documentation and clarifying builder naming.
- Add XML documentation comments to describe the component's purpose and usage.
- Consider renaming
__builder2to a more descriptive name likebuilderfor better clarity.src/LumexUI.Variants/CompoundVariant.cs (1)
7-12: Add XML documentation and consider making properties init-only.The class and its properties would benefit from XML documentation to explain their purpose and usage. Additionally, consider making the properties init-only to promote immutability.
Apply this diff to improve documentation and immutability:
+/// <summary> +/// Represents a compound variant that combines conditions with associated classes. +/// </summary> public class CompoundVariant { + /// <summary> + /// Gets or sets the conditions that determine when this variant applies. + /// </summary> - public Dictionary<string, string> Conditions { get; set; } = []; + public Dictionary<string, string> Conditions { get; init; } = []; + /// <summary> + /// Gets or sets the collection of classes to apply when conditions are met. + /// </summary> - public SlotCollection Classes { get; set; } = []; + public SlotCollection Classes { get; init; } = []; }src/LumexUI.Variants/Extensions/SeviceCollectionExtensions.cs (3)
11-15: Complete the XML documentation.The XML documentation for the
AddTailwindVariantsmethod is empty. Please add a description of what the method does and the purpose of theservicesparameter.
18-18: Remove or implement the commented code.The commented registration for
TwMergeshould either be implemented if needed or removed if not required.
1-21: Fix the filename typo.The filename contains a typo: "SeviceCollectionExtensions.cs" should be "ServiceCollectionExtensions.cs".
src/LumexUI/Extensions/ParameterViewExtensions.cs (1)
11-11: Justify the exclusion from code coverage.The
[ExcludeFromCodeCoverage]attribute suggests this utility method isn't being tested. Consider removing this attribute and adding unit tests to ensure reliability, as this method could be critical for parameter handling across components.Would you like me to help create unit tests for this extension method?
src/LumexUI.Variants/CompoundVariantCollection.cs (1)
9-23: LGTM! Clean collection implementation with modern C# features.The implementation is concise and correctly implements
IEnumerable<CompoundVariant>. Consider these potential enhancements for future iterations:
- Add methods like
Remove,Clear, etc., for better collection management- Consider implementing
ICollection<T>for richer collection functionality- Add capacity management for performance optimization
tests/LumexUI.Tests/Extensions/AngleSharpExtensions.cs (1)
11-21: Add logging and specific exception handling.The error handling could be improved in the following ways:
- Add logging for caught exceptions to aid debugging
- Catch specific exceptions instead of all exceptions
- Consider adding XML documentation to clarify the null return case
+using Microsoft.Extensions.Logging; internal static class AngleSharpExtensions { - public static IElement? FindByTestId( this IRenderedFragment fragment, string id ) + /// <summary> + /// Finds an element by its test ID. + /// </summary> + /// <returns>The element if found, null if not found or if an error occurs during search.</returns> + public static IElement? FindByTestId( this IRenderedFragment fragment, string id, ILogger? logger = null ) { try { return fragment.Find( $"[data-testid={id}]" ); } - catch + catch (Exception ex) when (ex is ArgumentException || ex is InvalidOperationException) { + logger?.LogDebug(ex, "Failed to find element with test ID: {Id}", id); return null; } }docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Examples/EmptyContent.razor (2)
5-9: Add accessibility attributes to improve user experience.Consider adding ARIA attributes to enhance accessibility:
<LumexButton Variant="@Variant.Outlined" OnClick="@TriggerAsync" - data-popoverref="@_dropdownId"> + data-popoverref="@_dropdownId" + aria-haspopup="true" + aria-expanded="false" + aria-controls="@_dropdownId"> Open Dropdown </LumexButton>
20-20: Consider using a GUID for the dropdown ID.Using a hardcoded ID might cause conflicts if multiple instances of this component are rendered. Consider generating a unique ID:
- private string _dropdownId = "dropdown-empty-content"; + private string _dropdownId = $"dropdown-empty-content-{Guid.NewGuid():N}";docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/Arrow.razor (1)
20-20: Generate a unique ID for the popover.Using a hardcoded ID could lead to conflicts if multiple instances of this component are used on the same page.
Apply this diff to generate a unique ID:
- private string _popoverId = "popover-arrow"; + private string _popoverId = $"popover-arrow-{Guid.NewGuid()}";src/LumexUI/Styles/Dropdown.cs (1)
15-15: Reconsider using a singleton pattern for the variant.The singleton pattern for
_variantcould be problematic if different variants are needed for different dropdown instances. Consider making this instance-specific or allowing variant overrides.src/LumexUI.Variants/VariantConfig.cs (1)
7-13: Add XML documentation for the record and its properties.Consider adding XML documentation to describe the purpose of the record and its properties, making it easier for other developers to understand their usage.
Add documentation like this:
+/// <summary> +/// Represents a configuration for component variants, including base styles, slots, and variant collections. +/// </summary> public record VariantConfig { + /// <summary> + /// Gets the base styles applied to the component. + /// </summary> public string? Base { get; init; } + /// <summary> + /// Gets the collection of slots for the component. + /// </summary> public SlotCollection? Slots { get; init; } + /// <summary> + /// Gets the collection of variants for the component. + /// </summary> public VariantCollection? Variants { get; init; } + /// <summary> + /// Gets the collection of compound variants for the component. + /// </summary> public CompoundVariantCollection? CompoundVariants { get; init; }docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/Colors.razor (2)
5-5: Consider documenting the reason for skipping the first enum value.The code skips the first value of
ThemeColorenum using[1..]. If this is intentionally skipping a default/none value, consider adding a comment explaining why.
26-29: Consider adding error handling for the async operation.The
TriggerAsyncmethod directly returns the service call without error handling. Consider wrapping it in a try-catch block to handle potential failures gracefully.private Task TriggerAsync( string id ) { - return PopoverService.TriggerAsync( id ); + try + { + return PopoverService.TriggerAsync( id ); + } + catch (Exception ex) + { + // Log error or show user-friendly message + throw; + } }docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Examples/Usage.razor (2)
11-20: Consider adding ARIA labels for better accessibility.The dropdown menu items could benefit from ARIA labels to improve accessibility.
-<LumexDropdownItem>New file</LumexDropdownItem> +<LumexDropdownItem aria-label="Create new file">New file</LumexDropdownItem> -<LumexDropdownItem>Edit file</LumexDropdownItem> +<LumexDropdownItem aria-label="Edit existing file">Edit file</LumexDropdownItem> -<LumexDropdownItem>Share file</LumexDropdownItem> +<LumexDropdownItem aria-label="Share file with others">Share file</LumexDropdownItem> -<LumexDropdownItem Color="@ThemeColor.Danger" Class="text-danger"> +<LumexDropdownItem Color="@ThemeColor.Danger" Class="text-danger" aria-label="Delete file permanently"> Delete file </LumexDropdownItem>
23-23: Consider using a constant for the dropdown ID.The dropdown ID is hardcoded as a string. Consider defining it as a constant to improve maintainability.
-private string _dropdownId = "dropdown-usage"; +private const string DropdownId = "dropdown-usage";src/LumexUI/Common/Enums/MenuVariant.cs (1)
7-11: Consider enhancing documentation with usage examples.The XML documentation is good but could be improved by adding code examples showing how to use each variant.
/// <summary> /// Specifies the visual variants for menu-like components, /// such as <see cref="LumexDropdown"/>, and others. +/// <example> +/// <code> +/// var menu = new LumexDropdown { Variant = MenuVariant.Solid }; +/// </code> +/// </example> /// </summary>docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/Shadows.razor (1)
9-14: Consider making button styling configurable.The button styling (success color and flat variant) is hardcoded. Consider making it configurable to improve reusability.
+@code { + [Parameter] + public ThemeColor ButtonColor { get; set; } = ThemeColor.Success; + + [Parameter] + public Variant ButtonVariant { get; set; } = Variant.Flat; +} -<LumexButton Color="@ThemeColor.Success" - Variant="@Variant.Flat" +<LumexButton Color="@ButtonColor" + Variant="@ButtonVariant"src/LumexUI.Variants/SlotCollection.cs (1)
1-34: Add XML documentation for public members.Please add XML documentation for the class and its public members to improve code maintainability and IntelliSense support.
Add documentation like this:
+/// <summary> +/// Represents a collection of slots and their associated values. +/// </summary> public class SlotCollection : IEnumerable<KeyValuePair<string, string>> { private readonly Dictionary<string, string?> _dictionary = []; + /// <summary> + /// Gets or sets the value associated with the specified slot name. + /// </summary> + /// <param name="slotName">The name of the slot.</param> + /// <returns>The value associated with the slot, or null if the slot doesn't exist.</returns> public string? this[string slotName]docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Examples/Disabled.razor (1)
16-20: Enhance accessibility for disabled menu item.Add ARIA attributes to improve accessibility for screen readers.
Apply this diff:
<LumexDropdownItem Disabled="@true" Color="@ThemeColor.Danger" - Class="text-danger"> + Class="text-danger" + aria-disabled="true" + aria-label="Delete file (disabled)"> Delete file </LumexDropdownItem>src/LumexUI.Variants/VariantValueCollection.cs (1)
35-35: Fix inconsistent line ending.Remove the extra blank line at the end of the file.
} -src/LumexUI.Variants/VariantCollection.cs (2)
35-35: Fix inconsistent line ending.Remove the extra blank line at the end of the file.
} -
11-11: Consider adding validation for variant names.The collection allows any string as a variant name without validation. Consider adding checks for null, empty, or whitespace names.
Example implementation:
+private void ValidateVariantName(string variantName) +{ + if (string.IsNullOrWhiteSpace(variantName)) + { + throw new ArgumentException("Variant name cannot be null or whitespace.", nameof(variantName)); + } +} public VariantValueCollection? this[string variantName] { get { + ValidateVariantName(variantName); _dictionary.TryGetValue(variantName, out var variantValues); return variantValues; } set { + ValidateVariantName(variantName); _dictionary[variantName] = value; } }Also applies to: 26-27
docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Examples/DisabledItems.razor (1)
11-22: Consider adding ARIA attributes for better accessibility.While the disabled items are correctly implemented using the
DisabledItemsproperty, consider enhancing accessibility by adding ARIA attributes.-<LumexDropdownMenu DisabledItems="@(["share", "delete"])"> +<LumexDropdownMenu DisabledItems="@(["share", "delete"])" + aria-label="File operations"> <LumexDropdownItem Id="@("new")">New file</LumexDropdownItem> <LumexDropdownItem Id="@("edit")">Edit file</LumexDropdownItem> - <LumexDropdownItem Id="@("share")">Share file</LumexDropdownItem> + <LumexDropdownItem Id="@("share")" + aria-disabled="true">Share file</LumexDropdownItem> - <LumexDropdownItem Id="@("delete")" - Color="@ThemeColor.Danger" - Class="text-danger"> + <LumexDropdownItem Id="@("delete")" + Color="@ThemeColor.Danger" + Class="text-danger" + aria-disabled="true"> Delete file </LumexDropdownItem>docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/Placements.razor (1)
5-27: Consider using CSS Grid areas for better layout control.While the current layout using flex and grid works, consider using CSS Grid areas for more precise control over the placement examples.
-<div class="flex flex-wrap md:inline-grid md:grid-cols-3 gap-4"> +<div class="grid grid-areas-placement gap-4">src/LumexUI/Components/Dropdown/LumexDropdown.razor.cs (1)
17-19: Consider initializing _slots in the constructor.The
_slotsfield is initialized using collection expression syntax, but it might be clearer to initialize it in the constructor along with other fields.-private Dictionary<string, ComponentSlot> _slots = []; +private Dictionary<string, ComponentSlot> _slots; public LumexDropdown() { _context = new DropdownContext(this); + _slots = new Dictionary<string, ComponentSlot>();src/LumexUI/Components/Menu/MenuSlots.cs (2)
21-22: Consider adding a migration guide for the deprecated Root slot.The
Rootproperty is marked as obsolete, but it would be helpful to provide more detailed migration instructions in the deprecation message.-[Obsolete("Deprecated. This will be removed in the future releases. Use the 'Base' slot instead.")] +[Obsolete("Deprecated. This will be removed in v2.0.0. Migrate to the 'Base' slot by replacing Root with Base in your slot assignments. For example: Classes.Root = 'my-class' becomes Classes.Base = 'my-class'.")]
27-37: Consider adding validation for slot class names.The slot properties accept any string value without validation. Consider adding validation to ensure class names follow CSS naming conventions.
+using System.Text.RegularExpressions; public class MenuSlots : ISlot { + private static readonly Regex ValidClassNamePattern = new("^[a-zA-Z0-9_-][a-zA-Z0-9_-]*$"); + private string? _base; + private string? _list; + private string? _emptyContent; + + private void ValidateClassName(string? value, string propertyName) + { + if (value is not null && !ValidClassNamePattern.IsMatch(value)) + { + throw new ArgumentException($"Invalid CSS class name: {value}", propertyName); + } + } + public string? Base { - get; set; + get => _base; + set + { + ValidateClassName(value, nameof(Base)); + _base = value; + } }src/LumexUI/Services/Popover/IPopoverService.cs (1)
35-40: Complete the XML documentation forTriggerAsync.The XML documentation for the
TriggerAsyncmethod is incomplete. Please add descriptions for the method, parameter, and return value.Apply this diff to improve the documentation:
/// <summary> -/// +/// Triggers the popover with the specified identifier. /// </summary> -/// <param name="id"></param> -/// <returns></returns> +/// <param name="id">The unique identifier of the popover to trigger.</param> +/// <returns>A task representing the asynchronous operation.</returns>src/LumexUI/Components/Dropdown/LumexDropdownItem.cs (1)
41-52: Consider using StateHasChanged instead of Rerender.The
Rerendercall after async operations might be better handled usingStateHasChanged(). This would be more idiomatic in Blazor and could prevent potential race conditions.Apply this diff to improve the implementation:
private protected override async Task OnClickAsync( MouseEventArgs args ) { if( _disabled || ReadOnly ) { return; } await Dropdown.HideAsync(); await OnClick.InvokeAsync( args ); - Dropdown.Rerender(); + StateHasChanged(); }src/LumexUI/Components/Menu/Menu.razor (2)
14-20: Follow consistent slot naming conventions.The slot names "Base" and "List" should follow a consistent naming pattern. Consider using kebab-case for all slot names to match the "empty-content" slot.
- <div class="@_slots["Base"]( Classes?.Base, Class )" + <div class="@_slots["base"]( Classes?.Base, Class )" style="@Style" data-slot="base" @attributes="@AdditionalAttributes"> <LumexComponent As="@As" - Class="@_slots["List"]( Classes?.List )" + Class="@_slots["list"]( Classes?.List )" data-slot="list">
35-41: Optimize rendering for large item collections.The current implementation iterates through all items synchronously. Consider implementing virtualization or pagination for better performance with large lists.
src/LumexUI/Components/Dropdown/LumexDropdownMenu.cs (2)
17-28: Enhance parameter documentation.The XML documentation for Classes and ItemClasses parameters should include examples and explain the inheritance relationship with the base Menu class.
46-51: Add error handling for BuildRenderTree.The BuildRenderTree method should include error handling for cases where the base.BuildRenderTree delegate fails.
protected override void BuildRenderTree( RenderTreeBuilder builder ) { builder.OpenComponent<LumexPopoverContent>( 0 ); - builder.AddComponentParameter( 1, nameof( ChildContent ), (RenderFragment)base.BuildRenderTree ); + builder.AddComponentParameter( 1, nameof( ChildContent ), (RenderFragment)(builderInner => + { + try + { + base.BuildRenderTree(builderInner); + } + catch (Exception ex) + { + // Log error and render fallback content + builderInner.AddMarkupContent(0, "<div class=\"error\">Failed to render menu content</div>"); + } + })); builder.CloseComponent(); }tests/LumexUI.Tests/Components/Card/CardTests.cs (1)
18-20: Add tests for variant functionality.With the addition of TwVariant service, consider adding tests to verify variant-specific behavior of the Card component.
[Fact] public void Card_WithVariant_ShouldRenderCorrectly() { var variant = "custom"; var component = RenderComponent<LumexCard>(p => p .Add(c => c.Variant, variant)); component.MarkupMatches($"<div class=\"custom-variant\">"); }docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Examples/StartEndContent.razor (1)
31-50: Refactor icon definitions for better maintainability.Consider extracting the icon definitions to a separate method or component to improve code organization and reusability.
+private RenderFragment CreateIcon(string icon, bool isDanger = false) => + @<LumexIcon Icon="@icon" + Size="@new("20")" + Class="@(isDanger ? "shrink-0" : "text-default-500 shrink-0")" />; -private RenderFragment _newFile = - @<LumexIcon Icon="@Icons.Rounded.LibraryAdd" - Size="@new("20")" - Class="text-default-500 shrink-0" />; +private RenderFragment _newFile = CreateIcon(Icons.Rounded.LibraryAdd);src/LumexUI/Components/Menu/MenuItem.razor (2)
4-6: Add error handling for context registration.Consider adding error handling to gracefully handle potential registration failures.
@{ + try { Context.Register( this ); + } + catch (Exception ex) { + // Log the error or handle it appropriately + Console.WriteLine($"Failed to register MenuItem: {ex.Message}"); + } }
9-13: Complete the XML documentation.The XML documentation is missing content. Please add a description of the method and its parameter.
/// <summary> -/// +/// Renders the menu item component using the provided RenderTreeBuilder. /// </summary> -/// <param name="__builder"></param> +/// <param name="__builder">The RenderTreeBuilder instance used to construct the component's render tree.</param>docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Examples/ColorsVariants.razor (1)
31-40: Consider adding a tooltip for color selection.The color selection UI could benefit from tooltips explaining the impact of each color on the dropdown's appearance.
- <LumexRadio Value="@color" Color="@color"> + <LumexRadio Value="@color" Color="@color" Title="@($"Apply {color} theme to dropdowns")">docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Examples/Description.razor (1)
35-53: Reduce code duplication in icon definitions.Consider extracting common icon properties into a reusable method or component to reduce duplication.
+ private RenderFragment CreateIcon(string icon, bool isDanger = false) => + @<LumexIcon Icon="@icon" + Size="@new("20")" + Class="@($"{(isDanger ? "" : "text-default-500")} shrink-0")" />; + private RenderFragment _newFile = - @<LumexIcon Icon="@Icons.Rounded.LibraryAdd" - Size="@new("20")" - Class="text-default-500 shrink-0" />; + CreateIcon(Icons.Rounded.LibraryAdd); // Apply similar changes to other icon definitionssrc/LumexUI/Components/Menu/Menu.razor.cs (1)
60-60: Consider using the new collection expression syntax.The empty dictionary initialization can be simplified using the new collection expression syntax.
-private Dictionary<string, ComponentSlot> _slots = []; +private Dictionary<string, ComponentSlot> _slots = {};src/LumexUI/Components/Popover/LumexPopoverTrigger.razor.cs (1)
17-17: Consider adding a migration guide link.The obsolete message should include a link to the migration guide or documentation explaining the new approach.
-[Obsolete("Deprecated. This will be removed in the future releases. See the Popover usage in the docs.")] +[Obsolete("Deprecated. This will be removed in the future releases. See the migration guide at https://lumexui.com/docs/migration/popover")]docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Examples/CustomStyles.razor (1)
34-41: Consider using LumexSelect for theme selection.The native select element breaks the consistent styling of the dropdown menu. Consider using the LumexSelect component for better UI consistency.
- <select id="theme" - name="theme" - class="z-10 outline-none w-16 py-0.5 rounded-md text-tiny group-hover:border-default-500 border border-default-300 bg-transparent text-default-500"> - <option>Light</option> - <option>Dark</option> - <option>System</option> - </select> + <LumexSelect Id="theme" + Size="@Size.Small" + Class="w-16"> + <LumexSelectItem Value="@("light")">Light</LumexSelectItem> + <LumexSelectItem Value="@("dark")">Dark</LumexSelectItem> + <LumexSelectItem Value="@("system")">System</LumexSelectItem> + </LumexSelect>docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Dropdown.razor (2)
71-72: Fix typo in the Custom Styles section.There's a typo in the word "supports".
- This component suppots named slots (represented as <code>data-*</code> attributes) that + This component supports named slots (represented as <code>data-*</code> attributes) that
108-116: Remove commented-out code.The commented-out DocsSlotsSection should be either implemented or removed to maintain code cleanliness.
-@* <DocsSlotsSection Slots="@_slots"> - <div> - <h4 class="font-semibold">Dropdown</h4> - <ul> - <li><Code>Class</Code></li> - <li><Code>Classes</Code></li> - </ul> - </div> -</DocsSlotsSection> *@tests/LumexUI.Tests/Components/Dropdown/DropdownTests.razor (1)
16-37: Enhance test coverage with specific assertions.While the test verifies basic rendering, consider adding more specific assertions to validate the rendered output, such as:
- Verify the presence of specific elements (button, menu).
- Check the correct nesting of components.
- Validate the data attributes.
[Fact] public void ShouldRenderCorrectly() { var id = "1"; - var action = () => Render( + var cut = Render( @<text> <LumexButton data-popoverref="@id"> Trigger </LumexButton> <LumexDropdown Id="@id"> <LumexDropdownMenu> <LumexDropdownItem>New file</LumexDropdownItem> <LumexDropdownItem>Edit file</LumexDropdownItem> <LumexDropdownItem>Share file</LumexDropdownItem> <LumexDropdownItem>Delete file</LumexDropdownItem> </LumexDropdownMenu> </LumexDropdown> </text> ); - action.Should().NotThrow(); + // Verify button + cut.Find("[data-popoverref]").Should().NotBeNull(); + + // Verify dropdown structure + var dropdown = cut.Find($"#{id}"); + dropdown.Should().NotBeNull(); + + // Verify menu items + var items = cut.FindAll("li"); + items.Should().HaveCount(4);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (95)
LumexUI.sln(3 hunks)docs/LumexUI.Docs.Client/Common/Navigation/NavigationStore.cs(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Dropdown.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Examples/ColorsVariants.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Examples/CustomStyles.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Examples/Description.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Examples/Disabled.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Examples/DisabledItems.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Examples/EmptyContent.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Examples/StartEndContent.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Examples/Usage.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/Dropdown/PreviewCodes/ColorsVariants.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/Dropdown/PreviewCodes/CustomStyles.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/Dropdown/PreviewCodes/Description.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/Dropdown/PreviewCodes/Disabled.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/Dropdown/PreviewCodes/DisabledItems.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/Dropdown/PreviewCodes/EmptyContent.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/Dropdown/PreviewCodes/StartEndContent.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/Dropdown/PreviewCodes/Usage.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/Arrow.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/Colors.razor(2 hunks)docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/CustomStyles.razor(2 hunks)docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/Offset.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/Placements.razor(2 hunks)docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/Shadows.razor(2 hunks)docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/Sizes.razor(2 hunks)docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/Usage.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/_Radius.razor(2 hunks)docs/LumexUI.Docs.Client/Pages/Components/Popover/Popover.razor(1 hunks)src/LumexUI.Variants/CompoundVariant.cs(1 hunks)src/LumexUI.Variants/CompoundVariantCollection.cs(1 hunks)src/LumexUI.Variants/Extensions/SeviceCollectionExtensions.cs(1 hunks)src/LumexUI.Variants/LumexUI.Variants.csproj(1 hunks)src/LumexUI.Variants/SlotCollection.cs(1 hunks)src/LumexUI.Variants/TwVariant.cs(1 hunks)src/LumexUI.Variants/VariantCollection.cs(1 hunks)src/LumexUI.Variants/VariantConfig.cs(1 hunks)src/LumexUI.Variants/VariantValueCollection.cs(1 hunks)src/LumexUI/Common/Enums/MenuVariant.cs(1 hunks)src/LumexUI/Components/Bases/LumexComponentBase.cs(2 hunks)src/LumexUI/Components/Dropdown/DropdownContext.cs(1 hunks)src/LumexUI/Components/Dropdown/DropdownItemSlots.cs(1 hunks)src/LumexUI/Components/Dropdown/DropdownMenuSlots.cs(1 hunks)src/LumexUI/Components/Dropdown/LumexDropdown.razor(1 hunks)src/LumexUI/Components/Dropdown/LumexDropdown.razor.cs(1 hunks)src/LumexUI/Components/Dropdown/LumexDropdownItem.cs(1 hunks)src/LumexUI/Components/Dropdown/LumexDropdownMenu.cs(1 hunks)src/LumexUI/Components/Menu/Menu.razor(1 hunks)src/LumexUI/Components/Menu/Menu.razor.cs(1 hunks)src/LumexUI/Components/Menu/MenuContext.cs(1 hunks)src/LumexUI/Components/Menu/MenuItem.razor(1 hunks)src/LumexUI/Components/Menu/MenuItem.razor.cs(1 hunks)src/LumexUI/Components/Menu/MenuItemSlots.cs(1 hunks)src/LumexUI/Components/Menu/MenuSlots.cs(1 hunks)src/LumexUI/Components/Popover/LumexPopover.razor.cs(2 hunks)src/LumexUI/Components/Popover/LumexPopoverContent.razor(1 hunks)src/LumexUI/Components/Popover/LumexPopoverContent.razor.cs(1 hunks)src/LumexUI/Components/Popover/LumexPopoverTrigger.razor.cs(1 hunks)src/LumexUI/Components/Popover/PopoverSlots.cs(1 hunks)src/LumexUI/Extensions/ParameterViewExtensions.cs(1 hunks)src/LumexUI/Extensions/ServiceCollectionExtensions.cs(3 hunks)src/LumexUI/LumexUI.csproj(1 hunks)src/LumexUI/Services/Popover/IPopoverService.cs(1 hunks)src/LumexUI/Services/Popover/PopoverService.cs(2 hunks)src/LumexUI/Styles/Button.cs(1 hunks)src/LumexUI/Styles/Dropdown.cs(1 hunks)src/LumexUI/Styles/Menu.cs(1 hunks)src/LumexUI/Styles/Popover.cs(1 hunks)src/LumexUI/Theme/Colors/SemanticColors.cs(1 hunks)tests/LumexUI.Tests/Components/Accordion/AccordionTests.cs(2 hunks)tests/LumexUI.Tests/Components/Bases/InputBaseTests.cs(2 hunks)tests/LumexUI.Tests/Components/Button/ButtonTests.cs(2 hunks)tests/LumexUI.Tests/Components/Card/CardTests.cs(2 hunks)tests/LumexUI.Tests/Components/Checkbox/CheckboxGroupTests.cs(2 hunks)tests/LumexUI.Tests/Components/Checkbox/CheckboxTests.cs(2 hunks)tests/LumexUI.Tests/Components/Collapse/CollapseTests.cs(2 hunks)tests/LumexUI.Tests/Components/DataGrid/DataGridTests.razor(1 hunks)tests/LumexUI.Tests/Components/Divider/DividerTests.cs(2 hunks)tests/LumexUI.Tests/Components/Dropdown/DropdownTests.razor(1 hunks)tests/LumexUI.Tests/Components/Icon/IconTests.cs(2 hunks)tests/LumexUI.Tests/Components/Link/LinkTests.cs(2 hunks)tests/LumexUI.Tests/Components/Listbox/ListboxTests.razor(1 hunks)tests/LumexUI.Tests/Components/Navbar/NavbarTests.razor(1 hunks)tests/LumexUI.Tests/Components/Numbox/NumboxTests.razor(1 hunks)tests/LumexUI.Tests/Components/Popover/PopoverTests.razor(1 hunks)tests/LumexUI.Tests/Components/Providers/ThemeProviderTests.cs(1 hunks)tests/LumexUI.Tests/Components/RadioGroup/RadioGroupTests.cs(2 hunks)tests/LumexUI.Tests/Components/RadioGroup/RadioTests.cs(2 hunks)tests/LumexUI.Tests/Components/Select/SelectTests.razor(1 hunks)tests/LumexUI.Tests/Components/Switch/SwitchTests.cs(2 hunks)tests/LumexUI.Tests/Components/Tabs/TabsTests.razor(1 hunks)tests/LumexUI.Tests/Components/Textbox/TextboxTests.razor(1 hunks)tests/LumexUI.Tests/Extensions/AngleSharpExtensions.cs(1 hunks)tests/LumexUI.Tests/LumexUI.Tests.csproj(1 hunks)tests/LumexUI.Tests/_Imports.razor(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- docs/LumexUI.Docs.Client/Common/Navigation/NavigationStore.cs
- tests/LumexUI.Tests/_Imports.razor
- src/LumexUI.Variants/LumexUI.Variants.csproj
🧰 Additional context used
🪛 GitHub Check: build-test
src/LumexUI/Styles/Popover.cs
[warning] 45-45:
'PopoverSlots.Trigger' is obsolete: 'Deprecated. This will be removed in the future releases. Use the 'Base' slot instead.'
🪛 GitHub Check: codecov/patch
src/LumexUI/Components/Popover/LumexPopoverTrigger.razor.cs
[warning] 62-62: src/LumexUI/Components/Popover/LumexPopoverTrigger.razor.cs#L62
Added line #L62 was not covered by tests
[warning] 66-67: src/LumexUI/Components/Popover/LumexPopoverTrigger.razor.cs#L66-L67
Added lines #L66 - L67 were not covered by tests
src/LumexUI/Services/Popover/PopoverService.cs
[warning] 46-46: src/LumexUI/Services/Popover/PopoverService.cs#L46
Added line #L46 was not covered by tests
src/LumexUI/Components/Popover/LumexPopover.razor.cs
[warning] 141-141: src/LumexUI/Components/Popover/LumexPopover.razor.cs#L141
Added line #L141 was not covered by tests
[warning] 160-161: src/LumexUI/Components/Popover/LumexPopover.razor.cs#L160-L161
Added lines #L160 - L161 were not covered by tests
src/LumexUI/Components/Popover/LumexPopoverContent.razor.cs
[warning] 67-67: src/LumexUI/Components/Popover/LumexPopoverContent.razor.cs#L67
Added line #L67 was not covered by tests
🔇 Additional comments (106)
tests/LumexUI.Tests/Components/Select/SelectTests.razor (1)
12-12: LGTM! Service registration aligns with variant system integration.The singleton registration of
TwVariantis consistent with other service registrations and supports the new variant configuration system.tests/LumexUI.Tests/Components/DataGrid/DataGridTests.razor (2)
20-21: LGTM! Service registration aligns with variant system integration.The addition of
TwVariantservice registration alongsideTwMergeis consistent with the broader changes to integrate the variant configuration system.
29-731: Comprehensive test coverage maintained.The test suite provides thorough coverage of DataGrid functionality including:
- Basic rendering
- Empty/loading states
- Selection modes
- Sorting
- Editing
- Validation
- Error handling
The unchanged tests suggest backward compatibility with the variant system integration.
tests/LumexUI.Tests/Components/Textbox/TextboxTests.razor (1)
12-14: LGTM! Consider adding variant-specific test cases.The addition of the
TwVariantsingleton service aligns with the variant system integration. While the service registration is correct, consider adding test cases to verify the variant functionality of the Textbox component.Would you like me to help generate test cases for variant-specific scenarios?
tests/LumexUI.Tests/Components/Checkbox/CheckboxGroupTests.cs (2)
5-5: LGTM!The using statement is correctly placed and properly organized.
18-18: LGTM! Service registration aligns with the new variant system.The TwVariant service is correctly registered as a singleton, matching the lifetime scope of the existing TwMerge service.
tests/LumexUI.Tests/Components/Divider/DividerTests.cs (1)
6-6: LGTM!The import statement for
LumexUI.Variantsis correctly placed and necessary for theTwVariantservice registration.docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/CustomStyles.razor (3)
1-3: LGTM! Clean service integration.The service setup follows Blazor's dependency injection pattern correctly.
5-8: LGTM! Well-structured trigger implementation.The button implementation follows best practices with clear event handling and proper reference binding.
33-38: LGTM! Clean async implementation.The trigger method follows async/await best practices with proper encapsulation of the popover ID.
tests/LumexUI.Tests/Components/Icon/IconTests.cs (3)
5-6: LGTM! Using statement properly organized.The new using statement for LumexUI.Variants is correctly placed and follows the project's organization standards.
17-18: LGTM! Service registration properly configured.The TwVariant service is correctly registered as a singleton, matching the registration pattern used for TwMerge and aligning with the broader variant system integration across test files.
21-39: Consider adding variant-related test cases.With the introduction of the variant system, consider adding test cases that verify:
- Icon rendering with different variants
- Variant class application
- Variant style overrides
Would you like me to help generate additional test cases for variant-specific scenarios?
tests/LumexUI.Tests/Components/Switch/SwitchTests.cs (2)
16-20: Add test coverage for variant functionality.The
TwVariantservice has been added, but there are no tests verifying its integration with the Switch component.Consider adding test cases to verify:
- Default variant styling
- Custom variant application
- Variant overrides
22-136: Excellent test coverage for core functionality!The test suite provides comprehensive coverage of the Switch component's features:
- Basic rendering and child content
- Icon customization (thumb, start, end positions)
- Value changes and validation
- Disabled/ReadOnly states
- Async operations
tests/LumexUI.Tests/Components/Tabs/TabsTests.razor (1)
15-15: LGTM! Service registration follows the established pattern.The
TwVariantsingleton registration aligns with the existing service registration pattern and integrates well with the test setup.docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/Offset.razor (3)
1-3: LGTM! Clean service injection setup.The dependency injection follows Blazor best practices and aligns with the architectural shift to a service-based approach.
5-8: LGTM! Accessible button implementation.The button implementation follows accessibility best practices by using data attributes for reference binding, and the click handler is properly bound to an async method.
19-26: LGTM! Clean and focused implementation.The code-behind implementation follows C# best practices with proper async/await pattern usage and clear naming conventions.
tests/LumexUI.Tests/Components/Link/LinkTests.cs (2)
5-5: LGTM!The using statement for the Variants namespace is correctly placed and organized with other using statements.
18-18: LGTM!The TwVariant service is correctly registered as a singleton, consistent with the TwMerge registration pattern.
src/LumexUI/Theme/Colors/SemanticColors.cs (1)
39-40: Verify color contrast for accessibility.The change from Zinc-50/Zinc-700 to White/Zinc-900 increases contrast, which is good for accessibility. However, let's verify the contrast ratio meets WCAG guidelines.
#!/usr/bin/env python3 import math def luminance(r, g, b): rgb = [r/255, g/255, b/255] rgb = [x/12.92 if x <= 0.03928 else ((x+0.055)/1.055)**2.4 for x in rgb] return 0.2126 * rgb[0] + 0.7152 * rgb[1] + 0.0722 * rgb[2] def contrast_ratio(l1, l2): lighter = max(l1, l2) darker = min(l1, l2) return (lighter + 0.05) / (darker + 0.05) # White background (255, 255, 255) white_lum = luminance(255, 255, 255) # Zinc-900 foreground (24, 24, 27) - approximated values zinc900_lum = luminance(24, 24, 27) ratio = contrast_ratio(white_lum, zinc900_lum) print(f"Contrast ratio: {ratio:.2f}") print(f"Meets WCAG AA for normal text (4.5:1): {ratio >= 4.5}") print(f"Meets WCAG AAA for normal text (7:1): {ratio >= 7}")[skip_cloning]
tests/LumexUI.Tests/Components/Collapse/CollapseTests.cs (1)
5-5: LGTM!The new using directive is correctly placed and necessary for the
TwVariantservice.src/LumexUI/LumexUI.csproj (1)
61-64: New Project Reference Added for LumexUI.Variants.The addition of
<ProjectReference Include="..\LumexUI.Variants\LumexUI.Variants.csproj" />integrates the new Variants project as intended. Please verify that the relative path is correct with respect to the current project file location.tests/LumexUI.Tests/Components/Numbox/NumboxTests.razor (1)
13-14:✅ Verification successful
LGTM! Verify broader integration with variant system.
The addition of the TwVariant service registration is consistent with the broader changes for variant support across components.
Let's verify the integration with other test files:
🏁 Script executed:
#!/bin/bash # Description: Verify consistent TwVariant registration across test files # Search for TwVariant service registration in test files rg -l "Services\.AddSingleton<TwVariant>\(\)" "tests/LumexUI.Tests/Components" # Search for any test files that might be missing the registration fd -e razor$ . "tests/LumexUI.Tests/Components" --exec grep -L "Services\.AddSingleton<TwVariant>"Length of output: 1408
TwVariant Registration Consistent Across All Test Files
The singleton registration for TwVariant is applied consistently in all component test files, including the one in NumboxTests.razor. No missing registrations were detected across the codebase.
tests/LumexUI.Tests/Components/Popover/PopoverTests.razor (2)
14-14: LGTM! Service registration for TwVariant is properly configured.The singleton registration for
TwVariantis correctly placed and follows the established pattern for service registration in the test context.
85-111: Verify if the commented-out test case is still relevant.This test appears to cover an important edge case for popover behavior when triggering an already shown popover. If this is still a valid scenario, consider either:
- Implementing and enabling the test
- Adding a comment explaining why it's disabled
- Removing it if no longer needed
docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/Usage.razor (1)
1-4: LGTM! Clean service injection implementation.The service-based approach follows Blazor best practices with proper dependency injection and clear separation of concerns.
tests/LumexUI.Tests/Components/RadioGroup/RadioGroupTests.cs (2)
22-320: Test coverage looks comprehensive!The test suite thoroughly covers various scenarios including:
- Basic rendering and value changes
- ReadOnly/Disabled states
- Boolean parsing
- Support for nullable value and reference types
The test methods are well-structured with clear assertions and error cases.
5-5:✅ Verification successful
Verify if TwMerge is still needed.
Both
TwMergeandTwVariantservices are registered. IfTwVariantis intended to replaceTwMerge, consider removing theTwMergeregistration to avoid potential confusion and redundancy.Run this script to check if
TwMergeis still used in the RadioGroup component:Also applies to: 18-20
🏁 Script executed:
#!/bin/bash # Search for TwMerge usage in RadioGroup-related files rg -l "TwMerge" --type cs | grep -i "radiogroup"Length of output: 217
Confirmed: TwMerge is still in use.
The search results indicate thatTwMergeis referenced not only in theRadioGroupTests.csfile but also inRadioTests.csand the main component fileLumexRadioGroup.razor.cs. Given these findings, both theTwMergeandTwVariantservices appear to be actively utilized. Please ensure that if a transition toward exclusively usingTwVariantis intended, all references toTwMergeare removed and refactored accordingly.
- Current Usage:
tests/LumexUI.Tests/Components/RadioGroup/RadioGroupTests.cstests/LumexUI.Tests/Components/RadioGroup/RadioTests.cssrc/LumexUI/Components/Radio/LumexRadioGroup.razor.csFor now, retaining both registrations is necessary until a comprehensive refactor is pursued.
docs/LumexUI.Docs.Client/Pages/Components/Dropdown/PreviewCodes/StartEndContent.razor (2)
1-1: LGTM! Appropriate render mode for interactive components.The
InteractiveWebAssemblyrender mode is well-suited for the dropdown component as it enables client-side interactivity.
3-5:✅ Verification successful
Verify the snippet path and consider improving maintainability.
The code looks good, but let's ensure the snippet path exists and consider using a constant for better maintainability.
Run this script to verify the snippet path:
Consider defining the snippet path as a constant to improve maintainability:
+@code { + private const string SnippetPath = "Dropdown.Code.StartEndContent"; +} -<PreviewCode Code="@new(name: null, snippet: "Dropdown.Code.StartEndContent")"> +<PreviewCode Code="@new(name: null, snippet: SnippetPath)">
🏁 Script executed:
#!/bin/bash # Description: Verify that the Dropdown.Code.StartEndContent snippet exists rg -l "Dropdown\.Code\.StartEndContent" --type-add 'razor:*.razor' --type razorLength of output: 167
Snippet path verified; consider a constant for improved maintainability.
The snippet path
"Dropdown.Code.StartEndContent"was confirmed to exist.The current inline usage is correct, but to enhance maintainability, consider defining the snippet path as a constant in the code block. For example:
+@code { + private const string SnippetPath = "Dropdown.Code.StartEndContent"; +} -<PreviewCode Code="@new(name: null, snippet: "Dropdown.Code.StartEndContent")"> +<PreviewCode Code="@new(name: null, snippet: SnippetPath)">tests/LumexUI.Tests/Components/Checkbox/CheckboxTests.cs (1)
22-135: Test coverage looks comprehensive!The test methods thoroughly cover component functionality, including rendering, interactions, state changes, and edge cases. The transition from
TwMergetoTwVariantdoesn't impact the test assertions as they focus on component behavior rather than styling implementation.src/LumexUI/Styles/Button.cs (2)
15-34: LGTM! Well-structured base styles implementation.The base styles are well-organized with clear separation of concerns (layout, typography, transitions, focus states). The use of ElementClass builder pattern makes the code maintainable and easy to understand.
108-121: LGTM! Well-structured composition of styles.The GetStyles method effectively combines all style components in a logical order, with proper handling of states and options.
docs/LumexUI.Docs.Client/Pages/Components/Dropdown/PreviewCodes/EmptyContent.razor (1)
1-1: LGTM! Appropriate rendering mode for interactive components.The
InteractiveWebAssemblyrender mode is well-suited for dropdown components that require client-side interactivity.tests/LumexUI.Tests/Components/Bases/InputBaseTests.cs (1)
7-7: LGTM!The using directive is correctly placed and necessary for the TwVariant service.
src/LumexUI/Components/Popover/LumexPopoverContent.razor (3)
4-9: LGTM! State management refactor improves code clarity.The refactoring from
Context.OwnertoPopoverproperty improves readability while maintaining the same functionality. The wrapper's positioning and z-index ensure proper stacking context.
15-21: LGTM! Slot-based styling enhances customization.The use of slots for styling provides better customization options while maintaining a clean separation of concerns.
22-26: LGTM! Arrow implementation follows best practices.The arrow implementation using slots allows for consistent styling and positioning.
src/LumexUI/Components/Popover/LumexPopoverContent.razor.cs (2)
20-31: LGTM! Well-structured component setup with proper dependency injection.The component is well-organized with clear parameter definitions and proper dependency injection.
69-88: LGTM! Robust disposal implementation with proper error handling.The disposal implementation properly handles resource cleanup and gracefully handles client disconnection scenarios.
tests/LumexUI.Tests/Components/RadioGroup/RadioTests.cs (2)
6-6: LGTM! Import added for variant support.The import is properly placed and necessary for the TwVariant service registration.
18-19:❓ Verification inconclusive
Verify if both TwMerge and TwVariant services are needed.
Both services are registered as singletons. Based on the AI summary, there seems to be a transition from TwMerge to TwVariant across the codebase. Please verify if both services are required during this transition period.
🏁 Script executed:
#!/bin/bash # Search for TwMerge usage in the codebase echo "Searching for TwMerge usage..." rg "TwMerge" -l echo -e "\nSearching for TwVariant usage..." rg "TwVariant" -lLength of output: 4996
Confirm Service Registrations and Standardize Indentation
- Both
TwMergeandTwVariantare registered as singletons. The codebase search shows that references to both services exist across multiple files, suggesting they are concurrently needed during the transition phase. Please verify whether maintaining both registrations is still intentional or if the legacy service (TwMerge) can be phased out.- There is an indentation inconsistency in the registration block (one line uses spaces while the other uses a tab). Please update the formatting to conform to the project's styling guidelines.
docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/_Radius.razor (3)
1-3: Good use of dependency injection and service import.Injecting
IPopoverServiceis consistent with the new popover architecture. This is a straightforward and effective approach to keep UI logic cleanly separated from the service layer.
16-16: Popover setup looks correct.Using the dynamic
idfor both button and popover ensures a direct reference link, aligning well with the new popover architecture.
27-30: Asynchronous signature is appropriate for service-based triggers.Returning the Task from
PopoverService.TriggerAsyncis concise. If adding further logic, mark the methodasyncand useawait. For now, this is sufficient.docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/Sizes.razor (2)
1-3: No issues detected with the addedusingstatement and service injection.
The import and injection ofIPopoverServiceare well-structured and align with service-oriented best practices.
5-16: Ensure compatibility with older .NET versions.
Enum.GetValues<TEnum>()is available starting from .NET 5. If you plan to support older versions, consider usingEnum.GetValues(typeof(Size))and casting toSize. Otherwise, this approach is concise and clear. Additionally, the dynamic runtime generation of popover elements via enumeration is a good pattern.Would you like a script to search your project files for
.csprojframeworks, to verify you’re already on .NET 5+?src/LumexUI/Services/Popover/PopoverService.cs (1)
35-36: Unregistration behavior looks fine.Removing a popover by
Idgracefully handles scenarios where theIdmight not exist, returning false without throwing. This logic is acceptable unless you need explicit feedback when removal fails.src/LumexUI.Variants/TwVariant.cs (2)
1-14: File header and delegates look good.The licensing information, namespaces, and delegate signatures are appropriate. No immediate issues or suggestions here.
102-107: Override retrieval is straightforward.
GetOverrideValuereturning an empty string as a fallback is logically consistent and succinct.src/LumexUI/Components/Menu/MenuItem.razor.cs (9)
1-14: File header and namespace usage.Licensing, using statements, and namespace definition follow a consistent pattern. Nothing problematic here.
15-39: Parameter definitions for content and ID.The usage of
[Parameter]forChildContent,StartContent,EndContent, andIdis clear. Generating unique IDs withIdentifier.New()is helpful, though you may want to confirm if collisions are possible across the application lifetime.
40-60: Variant and color defaults are well-defined.Providing default values (e.g.,
MenuVariant.SolidandThemeColor.Default) ensures consistent styling if parameters are not explicitly set by the consumer.
61-80: Clickable, disabled, and divider configuration look good.The design is flexible, giving users the option to mark items as read-only, disabled, or visually separated with a divider. The
OnClickcallback parameter aligns with standard Blazor patterns.
81-96: Constructor and_slotsinitialization.Defining
As = "li"in the constructor clarifies the default HTML tag for this component._slots = []is presumably a shorthand for an empty dictionary if you're targeting newer C# versions; verify it compiles as expected across your supported runtime environments.
98-108: Parameter fallback logic is elegant.Using
GetParameterPropertyto respect local parameter settings before falling back toMenuproperties is a solid approach.
110-115: Cascading context check is valid.Throwing an exception if
Contextis null ensures that the component is only used within a validMenucontext, preventing runtime confusion.
116-130: Slot-building logic is robust.Merging styles/slots with
TwVariantneatly encapsulates the styling concerns in one place. This approach is coherent with the library’s variant-based design.
132-133: Abstract click handler.Declaring
OnClickAsyncas abstract encourages derived classes to provide their specific click logic. This design fosters consistency across different menu items.src/LumexUI/Components/Popover/LumexPopover.razor.cs (1)
141-161: Increase test coverage for the parameter validation and popover toggling logic.
Lines 141 and 160-161 are not covered by tests. The exception path at line 141 (ID not set) and the logic at lines 160-161 (unsetting last shown popover) should be validated with unit tests to ensure code correctness and prevent regressions.Would you like assistance generating unit tests to cover these code paths?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 141-141: src/LumexUI/Components/Popover/LumexPopover.razor.cs#L141
Added line #L141 was not covered by tests
[warning] 160-161: src/LumexUI/Components/Popover/LumexPopover.razor.cs#L160-L161
Added lines #L160 - L161 were not covered by testsdocs/LumexUI.Docs.Client/Pages/Components/Dropdown/PreviewCodes/Usage.razor (1)
1-5: LGTM! Well-structured documentation example.The component follows the standard documentation pattern and correctly uses InteractiveWebAssembly for interactive functionality.
docs/LumexUI.Docs.Client/Pages/Components/Dropdown/PreviewCodes/Disabled.razor (1)
1-5: LGTM! Consistent with documentation standards.The component maintains consistency with other preview code files and correctly implements the disabled dropdown example.
docs/LumexUI.Docs.Client/Pages/Components/Dropdown/PreviewCodes/Description.razor (1)
1-5: LGTM! Maintains documentation consistency.The component follows the established pattern for preview code files and correctly implements the description example.
docs/LumexUI.Docs.Client/Pages/Components/Dropdown/PreviewCodes/CustomStyles.razor (1)
1-5: LGTM!The component is well-structured and follows the project's conventions. The use of
@rendermode InteractiveWebAssemblyis appropriate for interactive components.docs/LumexUI.Docs.Client/Pages/Components/Dropdown/PreviewCodes/DisabledItems.razor (1)
1-5: LGTM!The component follows the established pattern and maintains consistency with other preview files.
docs/LumexUI.Docs.Client/Pages/Components/Dropdown/PreviewCodes/ColorsVariants.razor (1)
1-5: LGTM!The component maintains consistency with other preview files and follows the established pattern.
src/LumexUI.Variants/CompoundVariant.cs (1)
1-4: LGTM!The license header is properly formatted and includes all necessary information.
src/LumexUI/Components/Dropdown/DropdownContext.cs (1)
9-12: LGTM! Well-structured implementation.The class follows best practices with appropriate access modifiers, immutability, and modern C# features.
src/LumexUI/Components/Dropdown/DropdownMenuSlots.cs (1)
11-16: LGTM! Well-documented type marker.The class appropriately inherits from
MenuSlotsand includes clear XML documentation.src/LumexUI/Components/Dropdown/DropdownItemSlots.cs (1)
11-16: LGTM! Well-documented type marker.The class appropriately inherits from
MenuItemSlotsand includes clear XML documentation.src/LumexUI/Extensions/ParameterViewExtensions.cs (1)
14-17: LGTM! Clean and type-safe parameter retrieval.The implementation is concise and provides a safe way to retrieve parameter values with fallback support.
docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Examples/EmptyContent.razor (1)
11-17: LGTM! Clean and focused empty content implementation.The dropdown structure is well-organized and provides a good example of empty content handling.
docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/Arrow.razor (2)
1-17: LGTM!The implementation correctly uses dependency injection for the popover service and properly associates the button with the popover using the data-popoverref attribute.
22-25: LGTM!The TriggerAsync method correctly delegates to the PopoverService.
src/LumexUI/Styles/Dropdown.cs (1)
17-27: LGTM!The base styles provide a good foundation for the dropdown component with appropriate width and padding settings.
src/LumexUI.Variants/VariantConfig.cs (1)
14-24: LGTM!The Deconstruct method is well-implemented, enabling pattern matching and destructuring of the record.
src/LumexUI/Components/Menu/MenuContext.cs (1)
10-37: LGTM!The overall implementation is clean and well-structured:
- Uses primary constructor for dependency injection
- Properly manages item collection lifecycle
- Provides clear API for registering items
docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/Colors.razor (1)
7-23: LGTM! Well-structured popover implementation.The implementation follows good practices:
- Unique ID generation for each color variant
- Proper data attribute linking button to popover
- Consistent styling and content structure
docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Examples/Usage.razor (1)
5-9: LGTM! Clean button implementation.The button implementation follows good practices:
- Uses outlined variant for visual distinction
- Properly links to dropdown via data attribute
- Clear, descriptive button text
src/LumexUI/Common/Enums/MenuVariant.cs (1)
11-37: LGTM! Well-structured and documented enum.The enum implementation follows best practices:
- Clear and descriptive variant names
- Comprehensive XML documentation for each value
- Logical organization of variants
docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/Shadows.razor (1)
27-30: Consider adding error handling for the async operation.Similar to Colors.razor, the
TriggerAsyncmethod should include error handling.docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Examples/DisabledItems.razor (2)
5-9: LGTM! Button implementation follows best practices.The button correctly uses
data-popoverrefto link with the dropdown and implements the click handler.
27-30: LGTM! Async method follows best practices.The
TriggerAsyncmethod correctly returns the task fromPopoverService.TriggerAsync.docs/LumexUI.Docs.Client/Pages/Components/Popover/Examples/Placements.razor (1)
30-33: LGTM! Async method follows best practices.The
TriggerAsyncmethod correctly returns the task fromPopoverService.TriggerAsync.src/LumexUI/Components/Dropdown/LumexDropdown.razor.cs (1)
31-44: Consider adding null check for TwVariant.The
OnParametersSetmethod usesTwVariantwithout checking for null, which could potentially cause issues.protected override void OnParametersSet() { base.OnParametersSet(); + if (TwVariant is null) + { + return; + } + var dropdown = Dropdown.Style(TwVariant); _slots = dropdown();src/LumexUI/Components/Menu/MenuItemSlots.cs (1)
11-43: LGTM!The
MenuItemSlotsclass is well-structured with:
- Clear XML documentation for all properties
- Proper obsolescence handling for the
Rootproperty- Consistent naming conventions for slot properties
src/LumexUI/Components/Popover/PopoverSlots.cs (1)
21-33: LGTM!The changes properly handle the transition from
RootandTriggerslots to the newBaseslot by:
- Adding clear obsolescence messages
- Introducing the new
Baseproperty with proper documentation- Maintaining backward compatibility
src/LumexUI/Components/Dropdown/LumexDropdownItem.cs (1)
16-39: LGTM!The implementation is well-structured with:
- Clear inheritance hierarchy and interface implementation
- Proper context validation
- Correct parameter handling
src/LumexUI/Components/Menu/MenuItem.razor (1)
13-49: LGTM! Well-structured component with flexible styling.The component implementation is well-organized with:
- Proper use of slots for styling customization
- Conditional rendering for description
- Appropriate handling of disabled and readonly states
- Event handling for click interactions
src/LumexUI/Components/Bases/LumexComponentBase.cs (1)
41-42:✅ Verification successful
Verify the impact of access modifier changes.
The access modifier change for
TwMergefromprivate protectedtointernaland the addition ofinternal TwVariantcould expose these properties to a broader scope.Run this script to check for potential misuse of these properties outside the intended scope:
🏁 Script executed:
#!/bin/bash # Description: Check for usage of TwMerge and TwVariant outside the LumexUI assembly # Search for TwMerge usage echo "Checking TwMerge usage..." rg "TwMerge" --type cs -C 3 # Search for TwVariant usage echo "Checking TwVariant usage..." rg "TwVariant" --type cs -C 3Length of output: 53004
Access Modifier Change Verified: No Unintended Exposure Detected
The verification confirms that the changes—from using a more restricted
private protectedmodifier tointernal—are not leading to unintended exposure. Searches for bothTwMergeandTwVariantindicate that they are only used within the LumexUI assembly (including components and tests), and no external misuse is evident.
- File Affected:
src/LumexUI/Components/Bases/LumexComponentBase.cs- Updates:
[Inject] internal TwMerge TwMerge { get; set; } = default!; [Inject] internal TwVariant TwVariant { get; set; } = default!;- Outcome: The broader internal access is appropriate given the scope of usage within the assembly. No changes are needed based on the current usage.
docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Examples/ColorsVariants.razor (2)
6-28: LGTM! Well-structured dropdown examples.The implementation effectively demonstrates different variants with:
- Unique ID generation for each dropdown
- Proper event handling
- Consistent color application
42-63: LGTM! Clean implementation of variant mapping.The code section is well-organized with:
- Clear variant mapping logic
- Proper async handling
- Good use of pattern matching
docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Examples/Description.razor (2)
5-9: LGTM! Clean button implementation.The button implementation is well-structured with proper event handling and data attributes.
55-60: LGTM! Clean async implementation.The dropdown handling is well-implemented with proper async/await pattern.
src/LumexUI/Components/Menu/Menu.razor.cs (1)
74-79: Verify the TwVariant initialization.The
OnParametersSetmethod initializes styles but doesn't handle potential null values or exceptions. Consider adding null checks and error handling.protected override void OnParametersSet() { + if (TwVariant is null) + { + throw new InvalidOperationException("TwVariant service is not initialized"); + } + var menu = Styles.Menu.Style(TwVariant); + if (menu is null) + { + throw new InvalidOperationException("Failed to initialize menu styles"); + } _slots = menu(); }src/LumexUI/Extensions/ServiceCollectionExtensions.cs (1)
31-33: LGTM! Clean integration of TailwindVariants.The addition of
AddTailwindVariants()is well-placed in the service registration chain and consistently applied across both overloads.Also applies to: 44-46
src/LumexUI/Components/Popover/LumexPopoverTrigger.razor.cs (1)
62-67: Add test coverage for click handling.The static analysis indicates that the click handling code paths are not covered by tests.
Could you add tests to cover:
- The disabled state early return
- The
OnClickevent invocation🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 62-62: src/LumexUI/Components/Popover/LumexPopoverTrigger.razor.cs#L62
Added line #L62 was not covered by tests
[warning] 66-67: src/LumexUI/Components/Popover/LumexPopoverTrigger.razor.cs#L66-L67
Added lines #L66 - L67 were not covered by teststests/LumexUI.Tests/Components/Button/ButtonTests.cs (1)
19-21: LGTM! Clean integration of TwVariant service.The TwVariant service registration is properly added to the test context.
docs/LumexUI.Docs.Client/Pages/Components/Dropdown/Examples/CustomStyles.razor (2)
5-9: LGTM! Button implementation looks good.The button is correctly configured with the Outlined variant and properly references the dropdown ID.
56-72: LGTM! Styling classes are well-organized.The styling classes are properly structured using PopoverSlots and DropdownItemSlots for consistent theming.
tests/LumexUI.Tests/Components/Navbar/NavbarTests.razor (1)
11-13: LGTM! Service registration follows the established pattern.The TwVariant service is correctly registered as a singleton, consistent with the TwMerge service registration.
LumexUI.sln (1)
33-34: LGTM! Project structure is correctly configured.The LumexUI.Variants project is properly added to the solution with correct configurations and nesting.
Also applies to: 69-72, 85-85
tests/LumexUI.Tests/Components/Providers/ThemeProviderTests.cs (1)
55-56: LGTM!The CSS variable values have been updated to match the new design specifications:
--lumex-surface1: 0 0% 100%(pure white)--lumex-surface1-foreground: 240 5.88% 10%(darker text for better contrast)tests/LumexUI.Tests/Components/Accordion/AccordionTests.cs (1)
19-20: LGTM!The changes enhance the test context by:
- Adding TwVariant service for variant functionality testing
- Maintaining JSInterop setup for scroll height functionality
docs/LumexUI.Docs.Client/Pages/Components/Dropdown/PreviewCodes/EmptyContent.razor
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/LumexUI.Variants/SlotCollection.cs(1 hunks)src/LumexUI.Variants/TwVariant.cs(1 hunks)src/LumexUI.Variants/VariantCollection.cs(1 hunks)src/LumexUI.Variants/VariantValueCollection.cs(1 hunks)src/LumexUI/LumexUI.csproj(1 hunks)src/LumexUI/Services/Popover/PopoverService.cs(2 hunks)src/LumexUI/Styles/Button.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/LumexUI.Variants/VariantValueCollection.cs
- src/LumexUI/Styles/Button.cs
- src/LumexUI/LumexUI.csproj
- src/LumexUI.Variants/SlotCollection.cs
- src/LumexUI.Variants/VariantCollection.cs
🧰 Additional context used
🪛 GitHub Check: build-test
src/LumexUI.Variants/TwVariant.cs
[warning] 45-45:
Dereference of a possibly null reference.
[warning] 45-45:
Dereference of a possibly null reference.
🔇 Additional comments (5)
src/LumexUI.Variants/TwVariant.cs (4)
1-8: LGTM!The file header, license information, and namespace organization follow standard C# conventions.
9-11: LGTM!The delegate declarations are well-designed with appropriate use of nullable types and flexible parameter handling.
13-14: LGTM!The class effectively uses C# 12's primary constructor feature for dependency injection of TwMerge.
102-116: LGTM!The helper methods are well-implemented:
GetOverrideValuehandles null checks appropriatelyCreateComponentSloteffectively uses modern C# features for string manipulation and array creationsrc/LumexUI/Services/Popover/PopoverService.cs (1)
12-12: Use a proper dictionary initializer instead of=[].In most C# versions,
=[]on a Dictionary isn't valid. If you're targeting C# 9 or higher, you can use= new();for a concise initializer. Otherwise, use= new Dictionary<string, LumexPopover>();to avoid compilation issues.
| if( !string.IsNullOrEmpty( selectedValue ) && | ||
| values[selectedValue] is SlotCollection variantSlots ) | ||
| { |
There was a problem hiding this comment.
Add null check for values to prevent potential NullReferenceException.
The current code could throw a NullReferenceException if values is null when accessing values[selectedValue].
Apply this diff to add the necessary null check:
if( !string.IsNullOrEmpty( selectedValue ) &&
+ values is not null &&
values[selectedValue] is SlotCollection variantSlots )📝 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.
| if( !string.IsNullOrEmpty( selectedValue ) && | |
| values[selectedValue] is SlotCollection variantSlots ) | |
| { | |
| if( !string.IsNullOrEmpty( selectedValue ) && | |
| values is not null && | |
| values[selectedValue] is SlotCollection variantSlots ) | |
| { |
🧰 Tools
🪛 GitHub Check: build-test
[warning] 45-45:
Dereference of a possibly null reference.
[warning] 45-45:
Dereference of a possibly null reference.
| public void Unregister( LumexPopover popover ) | ||
| { | ||
| _registeredPopovers.Remove( popover ); | ||
| _registeredPopovers.Remove( popover.Id ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add parameter validation and existence check.
The Unregister method needs additional validation:
- Add null check for the popover parameter
- Verify the Id exists before removal
Apply this diff to improve robustness:
public void Unregister( LumexPopover popover )
{
- _registeredPopovers.Remove( popover.Id );
+ ArgumentNullException.ThrowIfNull(popover);
+ if (!string.IsNullOrEmpty(popover.Id) && _registeredPopovers.ContainsKey(popover.Id))
+ {
+ _registeredPopovers.Remove(popover.Id);
+ }
}📝 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.
| public void Unregister( LumexPopover popover ) | |
| { | |
| _registeredPopovers.Remove( popover ); | |
| _registeredPopovers.Remove( popover.Id ); | |
| } | |
| public void Unregister( LumexPopover popover ) | |
| { | |
| ArgumentNullException.ThrowIfNull(popover); | |
| if (!string.IsNullOrEmpty(popover.Id) && _registeredPopovers.ContainsKey(popover.Id)) | |
| { | |
| _registeredPopovers.Remove(popover.Id); | |
| } | |
| } |
| public void Register( LumexPopover popover ) | ||
| { | ||
| _registeredPopovers.Add( popover ); | ||
| if( !string.IsNullOrEmpty( popover.Id ) ) | ||
| { | ||
| _registeredPopovers.Add( popover.Id, popover ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add parameter validation and error handling.
The Register method needs additional validation and error handling:
- Add null check for the popover parameter
- Handle duplicate Ids gracefully
- Add try-catch for dictionary Add operation
Apply this diff to improve robustness:
public void Register( LumexPopover popover )
{
+ ArgumentNullException.ThrowIfNull(popover);
if( !string.IsNullOrEmpty( popover.Id ) )
{
- _registeredPopovers.Add( popover.Id, popover );
+ try
+ {
+ _registeredPopovers[popover.Id] = popover;
+ }
+ catch (Exception ex)
+ {
+ // Log the error if logging is available
+ throw new InvalidOperationException($"Failed to register popover with Id: {popover.Id}", ex);
+ }
}
+ else
+ {
+ throw new ArgumentException("Popover Id cannot be null or empty", nameof(popover));
+ }
}| /// <inheritdoc /> | ||
| public Task TriggerAsync( string id ) | ||
| { | ||
| if( _registeredPopovers.TryGetValue( id, out var popover ) ) | ||
| { | ||
| return popover.TriggerAsync(); | ||
| } | ||
|
|
||
| return Task.CompletedTask; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve parameter validation and documentation.
The TriggerAsync method needs improvements:
- Add parameter validation
- Remove trailing semicolon after if block
- Add XML documentation for the return value
- Consider logging for non-existent ids
Apply this diff to improve the implementation:
/// <inheritdoc />
+/// <returns>
+/// A task that completes when the popover trigger operation is finished.
+/// Returns a completed task if the popover with the specified id is not found.
+/// </returns>
public Task TriggerAsync( string id )
{
+ ArgumentException.ThrowIfNullOrEmpty(id);
if( _registeredPopovers.TryGetValue( id, out var popover ) )
{
- return popover.TriggerAsync();
+ return popover.TriggerAsync();
}
+ // Consider adding logging here for non-existent ids
return Task.CompletedTask;
}📝 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.
| /// <inheritdoc /> | |
| public Task TriggerAsync( string id ) | |
| { | |
| if( _registeredPopovers.TryGetValue( id, out var popover ) ) | |
| { | |
| return popover.TriggerAsync(); | |
| } | |
| return Task.CompletedTask; | |
| } | |
| /// <inheritdoc /> | |
| /// <returns> | |
| /// A task that completes when the popover trigger operation is finished. | |
| /// Returns a completed task if the popover with the specified id is not found. | |
| /// </returns> | |
| public Task TriggerAsync( string id ) | |
| { | |
| ArgumentException.ThrowIfNullOrEmpty(id); | |
| if( _registeredPopovers.TryGetValue( id, out var popover ) ) | |
| { | |
| return popover.TriggerAsync(); | |
| } | |
| // Consider adding logging here for non-existent ids | |
| return Task.CompletedTask; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/LumexUI.Variants/SlotCollection.cs(1 hunks)src/LumexUI.Variants/VariantCollection.cs(1 hunks)src/LumexUI.Variants/VariantValueCollection.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/LumexUI.Variants/VariantValueCollection.cs
- src/LumexUI.Variants/VariantCollection.cs
🔇 Additional comments (5)
src/LumexUI.Variants/SlotCollection.cs (5)
1-4: LGTM!The license header is properly formatted and includes the current year and valid license URL.
9-10: LGTM!The class declaration and interface implementation are properly aligned with matching types.
19-20: LGTM!The properties safely expose read-only collections of slots and values.
22-26: LGTM!The enumerator implementations are correct and follow best practices.
11-11:❓ Verification inconclusive
Verify C# version compatibility.
The collection expression syntax
[]requires C# 12.0 or later. Please ensure your project's target framework supports this feature.Run this script to check the project's language version:
🏁 Script executed:
#!/bin/bash # Description: Check C# language version in project file fd -e csproj -x cat {} | grep -i "LangVersion\|TargetFramework"Length of output: 818
C# Version Compatibility & Target Frameworks
The collection expression syntax
[]for initializing the dictionary requires C# 12.0 or later. The project files indicate several targets:
- Projects targeting net9.0 should support C# 12 features by default.
- Projects targeting net8.0 may not automatically enable C# 12 unless an explicit LangVersion setting is provided.
Please verify that any project targeting net8.0 either sets its LangVersion to 12.0 (or later) or is updated to a framework that guarantees C# 12 support. This will ensure consistent behavior across your codebase.
| public string this[string slotName] | ||
| { | ||
| get => _dictionary[slotName]; | ||
| set => _dictionary[slotName] = value; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add parameter validation and error handling.
The indexer should validate the input parameter and handle missing slots gracefully.
Apply this diff to improve robustness:
public string this[string slotName]
{
- get => _dictionary[slotName];
- set => _dictionary[slotName] = value;
+ get
+ {
+ ArgumentNullException.ThrowIfNull(slotName);
+ return _dictionary.TryGetValue(slotName, out var value)
+ ? value
+ : throw new KeyNotFoundException($"Slot '{slotName}' not found.");
+ }
+ set
+ {
+ ArgumentNullException.ThrowIfNull(slotName);
+ ArgumentNullException.ThrowIfNull(value);
+ _dictionary[slotName] = value;
+ }
}📝 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.
| public string this[string slotName] | |
| { | |
| get => _dictionary[slotName]; | |
| set => _dictionary[slotName] = value; | |
| } | |
| public string this[string slotName] | |
| { | |
| get | |
| { | |
| ArgumentNullException.ThrowIfNull(slotName); | |
| return _dictionary.TryGetValue(slotName, out var value) | |
| ? value | |
| : throw new KeyNotFoundException($"Slot '{slotName}' not found."); | |
| } | |
| set | |
| { | |
| ArgumentNullException.ThrowIfNull(slotName); | |
| ArgumentNullException.ThrowIfNull(value); | |
| _dictionary[slotName] = value; | |
| } | |
| } |
Description
Closes #115
Introducing a new dropdown component.
What's been done?
Added a new dropdown component.
Checklist
Additional Notes
Summary by CodeRabbit
New Features
TwVariantservice for improved component styling and behavior.Documentation