-
-
Notifications
You must be signed in to change notification settings - Fork 234
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
Add BitCollapse component (#9695) #9722
base: develop
Are you sure you want to change the base?
Add BitCollapse component (#9695) #9722
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 WalkthroughThe pull request introduces a new Changes
Assessment against linked issues
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 (
|
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: 0
🧹 Nitpick comments (10)
src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Collapse/BitCollapse.razor (2)
10-13
: Enhance accessibility attributes for the collapsible region.The
role="region"
is good, but additional ARIA attributes would improve accessibility:<div role="region" + aria-expanded="@Expanded" style="@Styles?.Content" class="bit-col-con @(Expanded ? "bit-col-cex" : "bit-col-cco") @Classes?.Content" tabindex="@(IsEnabled ? 0 : -1)">
4-9
: Consider adding keyboard interaction support.The root element should handle keyboard events (Enter/Space) to toggle the collapse state when focused.
<div @ref="RootElement" @attributes="HtmlAttributes" id="@_Id" style="@StyleBuilder.Value" class="@ClassBuilder.Value" dir="@Dir?.ToString().ToLower()" - aria-label="@AriaLabel"> + aria-label="@AriaLabel" + @onkeydown="@OnKeyDown" + role="button">src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Collapse/BitCollapseClassStyles.cs (1)
3-19
: Consider adding validation and helper methods.The class could benefit from:
- Method to validate CSS class names
- Helper method to combine multiple classes
- Constructor with default values
public class BitCollapseClassStyles { + public BitCollapseClassStyles() + { + Root = string.Empty; + Expanded = string.Empty; + Content = string.Empty; + } + + private static string ValidateCssClass(string? value) + { + if (string.IsNullOrWhiteSpace(value)) return string.Empty; + // Add CSS class validation logic if needed + return value; + } + /// <summary> /// Custom CSS classes/styles for the root element of the BitCollapse. /// </summary> - public string? Root { get; set; } + public string? Root + { + get => _root; + set => _root = ValidateCssClass(value); + } + private string? _root;src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Collapse/BitCollapse.razor.cs (1)
21-22
: Add event callback for state changes.Consider adding an event callback to notify parent components of state changes.
[Parameter, ResetClassBuilder, ResetStyleBuilder] public bool Expanded { get; set; } + +[Parameter] +public EventCallback<bool> ExpandedChanged { get; set; }src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Surfaces/Collapse/BitCollapseDemo.razor.cs (2)
77-80
: Add state change error handling.Consider demonstrating error handling for state changes in the demo.
private void OnExpandCollapseClick() { - expanded = !expanded; + try + { + expanded = !expanded; + StateHasChanged(); + } + catch (Exception ex) + { + // Demo error handling + Console.WriteLine($"Error toggling state: {ex.Message}"); + } }
5-39
: Add performance optimization examples.Consider adding examples demonstrating performance optimization techniques:
- Lazy loading of content
- Virtualization for large content
- Debounced collapse/expand
src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Surfaces/Collapse/BitCollapseDemo.razor.samples.cs (1)
17-32
: Consider using CSS variables for better maintainability.While the example effectively demonstrates both inline styles and CSS classes, consider:
- Using CSS variables instead of hardcoded colors (e.g.,
var(--bit-primary-color)
instead of#0054C6
)- Preferring CSS classes over inline styles for better maintainability
- background-color: #333; + background-color: var(--bit-background-color); - border: 1px solid #ff0000; + border: 1px solid var(--bit-error-color);src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Surfaces/Collapse/BitCollapseDemo.razor (1)
22-49
: Consider extracting repeated Lorem ipsum text to a constant.The examples contain duplicate Lorem ipsum text. Consider extracting it to a constant in the code-behind file to improve maintainability.
+private const string LoremIpsumText = "Lorem ipsum dolor sit amet..."; -Lorem ipsum dolor sit amet, consectetur adipiscing elit... +@LoremIpsumTextsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Surfaces/Collapse/BitCollapseDemo.razor.scss (1)
4-8
: Use theme variables for consistent styling.Replace hardcoded color values with theme variables to maintain consistency with the design system.
.custom-expanded { padding: 10px; - background-color: #808080; - border: 1px solid #0054C6; + background-color: $clr-bg-sec; + border: 1px solid $clr-pri; }src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Collapse/BitCollapse.scss (1)
22-22
: Consider making transition duration configurable.The transition duration is hardcoded to 300ms. Consider making it configurable through a CSS variable to allow customization.
- transition: all 300ms cubic-bezier(0.4, 0, 0.2, 1) 0ms; + transition: all var(--bit-collapse-transition-duration, 300ms) cubic-bezier(0.4, 0, 0.2, 1) 0ms;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Collapse/BitCollapse.razor
(1 hunks)src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Collapse/BitCollapse.razor.cs
(1 hunks)src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Collapse/BitCollapse.scss
(1 hunks)src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Collapse/BitCollapseClassStyles.cs
(1 hunks)src/BlazorUI/Bit.BlazorUI/Styles/components.scss
(1 hunks)src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Surfaces/Collapse/BitCollapseDemo.razor
(1 hunks)src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Surfaces/Collapse/BitCollapseDemo.razor.cs
(1 hunks)src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Surfaces/Collapse/BitCollapseDemo.razor.samples.cs
(1 hunks)src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Surfaces/Collapse/BitCollapseDemo.razor.scss
(1 hunks)src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Home/ComponentsSection.razor
(2 hunks)src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Shared/MainLayout.razor.NavItems.cs
(1 hunks)src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/compilerconfig.json
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build and test
🔇 Additional comments (10)
src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Collapse/BitCollapse.razor.cs (1)
33-38
: Consider adding transition completion handling.The CSS class registration looks good, but consider adding transition end event handling for better animation control.
src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Surfaces/Collapse/BitCollapseDemo.razor.samples.cs (2)
5-15
: LGTM! Clear and effective basic example.The example effectively demonstrates the core functionality of the BitCollapse component with a toggle button and state management.
34-44
: LGTM! Well-implemented RTL support.The example properly demonstrates RTL functionality with appropriate Persian text and consistent toggle behavior.
src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Surfaces/Collapse/BitCollapseDemo.razor (2)
1-10
: LGTM! Clear component metadata and description.The component's route, metadata, and description are well-structured and follow the established pattern.
11-20
: LGTM! Well-structured basic example implementation.The example effectively demonstrates the component's usage with proper component composition.
src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Shared/MainLayout.razor.NavItems.cs (1)
121-121
: LGTM! Properly integrated navigation item.The Collapse component is correctly added to the Surfaces section, maintaining alphabetical order and following the established pattern.
src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Home/ComponentsSection.razor (1)
188-190
: LGTM! Consistent component link implementation.The Collapse component link is properly integrated into the Surfaces section, following the established pattern and styling.
src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Collapse/BitCollapse.scss (1)
28-28
: Review margin compensation approach.Using negative margins to compensate for padding might cause layout shifts. Consider using
height: 0
andpadding: 0
in the collapsed state instead..bit-col-cco { opacity: 0; max-height: 0; - margin: spacing(-1.5) 0; + padding: 0; }src/BlazorUI/Bit.BlazorUI/Styles/components.scss (1)
71-71
: LGTM!The BitCollapse component import is correctly placed in the Surfaces section, maintaining the established organization pattern.
src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/compilerconfig.json (1)
368-373
: LGTM!The compiler configuration for BitCollapseDemo follows the established pattern and maintains consistency with other component configurations.
This closes #9695
Summary by CodeRabbit
New Features
Documentation
Styles