feat: Added ShadBreadcrumb #388
Conversation
Adds a new breadcrumb component with item, link, separator and ellipsis widgets. Includes theming support and example implementation.
Adds a new page showcasing various breadcrumb implementations. Includes examples with simple text, links, ellipsis, custom separators, and different alignments. This allows developers to easily visualize and implement breadcrumbs in their applications.
Adds spacing customization to breadcrumbs via theme. Reduces the default size of separator and ellipsis icons for a cleaner look.
Implements a new breadcrumb component with item, link, and ellipsis variations. Provides comprehensive widget tests for the component, covering basic rendering, custom separators, text direction, alignment, link functionality, disabled states, cursor handling, ellipsis rendering, integration, theme spacing, and edge cases (empty and single children lists). Also adds golden tests for visual verification.
Replaces `ShadBreadcrumbItem` with `ShadBreadcrumbPage` for current page indication. This change simplifies the breadcrumb component by introducing a dedicated `ShadBreadcrumbPage` widget to represent the current page in the breadcrumb trail. This eliminates the need for the `isCurrentPage` property on the `ShadBreadcrumbItem`, leading to a cleaner and more intuitive API. Additionally, this commit corrects the default text color for breadcrumb links by removing the muted foreground color and relying on the default text style.
WalkthroughA new breadcrumb navigation component suite was introduced, including widgets, theming support, and integration into the UI framework. Example and playground pages demonstrate usage, and routing was updated to include the new page. Comprehensive widget tests were added, and the README checklist now marks the breadcrumb component as complete. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant Breadcrumb
participant Theme
User->>App: Navigates to Breadcrumbs Page
App->>Breadcrumb: Builds breadcrumb widget tree
Breadcrumb->>Theme: Retrieves breadcrumb theming
Theme-->>Breadcrumb: Returns styles, icons, spacing
Breadcrumb-->>App: Renders breadcrumb UI
User->>Breadcrumb: Taps breadcrumb link or dropdown
Breadcrumb-->>App: Executes navigation callback or shows dropdown
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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.
Actionable comments posted: 2
🔭 Outside diff range comments (4)
lib/src/theme/data.dart (4)
312-417: Critical: Missing breadcrumbTheme in lerp method.The static lerp method is missing breadcrumbTheme interpolation, which will cause issues during theme transitions. All other theme properties are interpolated, but breadcrumbTheme is omitted.
Add the missing breadcrumbTheme interpolation:
static ShadThemeData lerp(ShadThemeData a, ShadThemeData b, double t) { if (identical(a, b)) { return a; } return ShadThemeData( colorScheme: ShadColorScheme.lerp(a.colorScheme, b.colorScheme, t), brightness: b.brightness, + breadcrumbTheme: ShadBreadcrumbTheme.lerp(a.breadcrumbTheme, b.breadcrumbTheme, t), primaryButtonTheme: ShadButtonTheme.lerp(a.primaryButtonTheme, b.primaryButtonTheme, t), // ... rest of the interpolations
420-476: Critical: Missing breadcrumbTheme in equality operator.The equality operator is missing breadcrumbTheme comparison, which will cause incorrect equality results when comparing ShadThemeData instances.
Add the missing breadcrumbTheme comparison:
@override bool operator ==(Object other) { if (identical(this, other)) return true; return other is ShadBaseTheme && other.colorScheme == colorScheme && other.brightness == brightness && + other.breadcrumbTheme == breadcrumbTheme && other.primaryButtonTheme == primaryButtonTheme && // ... rest of the comparisons
479-532: Critical: Missing breadcrumbTheme in hashCode.The hashCode method is missing breadcrumbTheme, which will cause issues when using ShadThemeData instances in collections like Sets or as Map keys.
Add the missing breadcrumbTheme to the hash computation:
@override int get hashCode { return colorScheme.hashCode ^ brightness.hashCode ^ + breadcrumbTheme.hashCode ^ primaryButtonTheme.hashCode ^ // ... rest of the hash codes
534-649: Critical: Missing breadcrumbTheme in copyWith method.The copyWith method is missing breadcrumbTheme parameter, preventing users from creating a copy of the theme with a modified breadcrumb theme.
Add the missing breadcrumbTheme parameter:
ShadThemeData copyWith({ ShadColorScheme? colorScheme, + ShadBreadcrumbTheme? breadcrumbTheme, ShadButtonTheme? primaryButtonTheme, // ... rest of parameters }) { return ShadThemeData( colorScheme: colorScheme ?? this.colorScheme, + breadcrumbTheme: breadcrumbTheme ?? this.breadcrumbTheme, extensions: extensions ?? this.extensions, // ... rest of assignments
🧹 Nitpick comments (1)
lib/src/components/breadcrumb.dart (1)
283-296: Consider flexible sizing for better theme compatibilityThe fixed 36x36 size may not adapt well to different theme scales or accessibility requirements.
Consider using theme-based sizing:
return SizedBox( - width: 36, - height: 36, + width: breadcrumbTheme.ellipsisSize?.width ?? 36, + height: breadcrumbTheme.ellipsisSize?.height ?? 36, child: Center(Or use a more flexible approach based on icon size and padding.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
example/lib/pages/breadcrumb_example.dart(1 hunks)lib/shadcn_ui.dart(2 hunks)lib/src/components/breadcrumb.dart(1 hunks)lib/src/theme/components/breadcrumb.dart(1 hunks)lib/src/theme/data.dart(4 hunks)lib/src/theme/themes/base.dart(4 hunks)lib/src/theme/themes/default_theme_no_secondary_border_variant.dart(2 hunks)lib/src/theme/themes/default_theme_variant.dart(2 hunks)playground/lib/pages/breadcrumbs.dart(1 hunks)playground/lib/router.dart(2 hunks)test/src/components/breadcrumb_test.dart(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
lib/src/theme/themes/base.dart (1)
Learnt from: nank1ro
PR: nank1ro/flutter-shadcn-ui#352
File: docs/src/content/docs/index.md:129-135
Timestamp: 2025-05-13T09:57:53.516Z
Learning: The shadcn_ui package uses `WidgetStatePropertyAll` in theme configurations (such as `ScrollbarThemeData`) instead of Flutter's native `MaterialStatePropertyAll`. This is intentional and correct according to the package maintainer.
lib/shadcn_ui.dart (1)
Learnt from: nank1ro
PR: nank1ro/flutter-shadcn-ui#352
File: docs/src/content/docs/index.md:129-135
Timestamp: 2025-05-13T09:57:53.516Z
Learning: The shadcn_ui package uses `WidgetStatePropertyAll` in theme configurations (such as `ScrollbarThemeData`) instead of Flutter's native `MaterialStatePropertyAll`. This is intentional and correct according to the package maintainer.
lib/src/theme/themes/default_theme_variant.dart (1)
Learnt from: nank1ro
PR: nank1ro/flutter-shadcn-ui#352
File: docs/src/content/docs/index.md:129-135
Timestamp: 2025-05-13T09:57:53.516Z
Learning: The shadcn_ui package uses `WidgetStatePropertyAll` in theme configurations (such as `ScrollbarThemeData`) instead of Flutter's native `MaterialStatePropertyAll`. This is intentional and correct according to the package maintainer.
lib/src/theme/themes/default_theme_no_secondary_border_variant.dart (1)
Learnt from: nank1ro
PR: nank1ro/flutter-shadcn-ui#352
File: docs/src/content/docs/index.md:129-135
Timestamp: 2025-05-13T09:57:53.516Z
Learning: The shadcn_ui package uses `WidgetStatePropertyAll` in theme configurations (such as `ScrollbarThemeData`) instead of Flutter's native `MaterialStatePropertyAll`. This is intentional and correct according to the package maintainer.
lib/src/theme/data.dart (1)
Learnt from: nank1ro
PR: nank1ro/flutter-shadcn-ui#352
File: docs/src/content/docs/index.md:129-135
Timestamp: 2025-05-13T09:57:53.516Z
Learning: The shadcn_ui package uses `WidgetStatePropertyAll` in theme configurations (such as `ScrollbarThemeData`) instead of Flutter's native `MaterialStatePropertyAll`. This is intentional and correct according to the package maintainer.
playground/lib/pages/breadcrumbs.dart (1)
Learnt from: nank1ro
PR: nank1ro/flutter-shadcn-ui#352
File: docs/src/content/docs/index.md:129-135
Timestamp: 2025-05-13T09:57:53.516Z
Learning: The shadcn_ui package uses `WidgetStatePropertyAll` in theme configurations (such as `ScrollbarThemeData`) instead of Flutter's native `MaterialStatePropertyAll`. This is intentional and correct according to the package maintainer.
lib/src/components/breadcrumb.dart (1)
Learnt from: nank1ro
PR: nank1ro/flutter-shadcn-ui#352
File: docs/src/content/docs/index.md:129-135
Timestamp: 2025-05-13T09:57:53.516Z
Learning: The shadcn_ui package uses `WidgetStatePropertyAll` in theme configurations (such as `ScrollbarThemeData`) instead of Flutter's native `MaterialStatePropertyAll`. This is intentional and correct according to the package maintainer.
lib/src/theme/components/breadcrumb.dart (1)
Learnt from: nank1ro
PR: nank1ro/flutter-shadcn-ui#352
File: docs/src/content/docs/index.md:129-135
Timestamp: 2025-05-13T09:57:53.516Z
Learning: The shadcn_ui package uses `WidgetStatePropertyAll` in theme configurations (such as `ScrollbarThemeData`) instead of Flutter's native `MaterialStatePropertyAll`. This is intentional and correct according to the package maintainer.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Flutter test
🔇 Additional comments (26)
playground/lib/router.dart (2)
6-6: LGTM: Import follows alphabetical ordering convention.The breadcrumbs page import is correctly placed in alphabetical order with other page imports.
80-83: LGTM: Route configuration follows established pattern.The breadcrumbs route implementation follows the same pattern as other static pages in the router, with a clear path and direct widget instantiation.
lib/shadcn_ui.dart (2)
11-11: LGTM: Component export properly positioned.The breadcrumb component export is correctly placed in alphabetical order within the Components section.
81-81: LGTM: Theme export properly positioned.The breadcrumb theme export is correctly placed in alphabetical order within the Component Themes section.
lib/src/theme/themes/base.dart (4)
7-7: LGTM: Import follows alphabetical ordering convention.The breadcrumb theme import is correctly positioned in alphabetical order with other theme component imports.
56-56: LGTM: Constructor parameter properly positioned.The breadcrumbTheme parameter is correctly placed in alphabetical order within the ShadBaseTheme constructor parameters.
112-112: LGTM: Property properly positioned.The breadcrumbTheme property is correctly placed in alphabetical order within the ShadBaseTheme class properties.
169-169: LGTM: Abstract method properly positioned.The breadcrumbTheme() method is correctly placed in alphabetical order within the ShadThemeVariant abstract class methods.
lib/src/theme/themes/default_theme_no_secondary_border_variant.dart (2)
14-14: LGTM: Import follows alphabetical ordering convention.The breadcrumb theme import is correctly positioned in alphabetical order with other theme component imports.
241-270: LGTM: Comprehensive breadcrumb theme implementation.The breadcrumbTheme() method provides a well-structured theme configuration with:
- Appropriate icons using LucideIcons (chevronRight for separator, ellipsis for ellipsis)
- Consistent color scheme usage (mutedForeground for icons, foreground for text)
- Proper text style hierarchy with hover states and current page differentiation
- Sensible spacing and alignment values
The implementation follows the established pattern used by other theme methods in this class.
lib/src/theme/themes/default_theme_variant.dart (2)
14-14: LGTM: Import follows alphabetical ordering convention.The breadcrumb theme import is correctly positioned in alphabetical order with other theme component imports.
220-249: LGTM: Consistent breadcrumb theme implementation.The breadcrumbTheme() method provides identical configuration to the no-secondary-border variant, ensuring consistency across theme variants. The implementation includes:
- Proper icon usage with LucideIcons
- Consistent color scheme application
- Comprehensive text styling with appropriate hover and current page states
- Matching spacing and alignment values
This consistency between theme variants is essential for maintaining a cohesive user experience.
lib/src/theme/data.dart (4)
9-9: LGTM! Proper import for breadcrumb theme component.The import follows the established pattern for theme component imports.
61-61: LGTM! Breadcrumb theme parameter follows established pattern.The optional breadcrumbTheme parameter is correctly positioned and follows the same pattern as other theme parameters.
195-196: LGTM! Proper merging logic for breadcrumb theme.The mergeWith call follows the established pattern used by other theme components.
269-269: LGTM! Correctly added to private constructor.The breadcrumbTheme parameter is properly added to the private constructor following the established pattern.
playground/lib/pages/breadcrumbs.dart (1)
1-177: LGTM! Well-structured breadcrumb demonstration page.This playground page effectively demonstrates various breadcrumb patterns including:
- Simple static breadcrumbs
- Interactive breadcrumbs with links
- Breadcrumbs with ellipsis for truncation
- Custom separators
- Different alignment options
The code is well-organized with clear section headers and appropriate use of print statements for demo navigation callbacks.
example/lib/pages/breadcrumb_example.dart (2)
1-105: LGTM! Clean breadcrumb example with good structure.The example demonstrates three key breadcrumb patterns with clear organization and appropriate navigation callbacks for demonstration purposes.
41-41: No action needed—both APIs are supported and equivalent.ShadBreadcrumbPage is simply a convenience widget that wraps ShadBreadcrumbItem with
isCurrentPage: true. You can either:
- Use
ShadBreadcrumbPage(…)(as in the playground), or- Pass
isCurrentPage: truedirectly toShadBreadcrumbItem(as in this example).Both approaches yield identical behavior and are fully supported.
test/src/components/breadcrumb_test.dart (1)
1-503: LGTM! Comprehensive test suite with excellent coverage.This test file provides thorough coverage of the breadcrumb components including:
- Basic rendering tests: Verifies all components render correctly
- Configuration tests: Custom separators, text direction, alignment
- Interaction tests: Link taps, disabled states, cursor handling
- Integration tests: Complex breadcrumb combinations
- Edge case handling: Empty children, single items
- Visual regression: Golden tests for UI consistency
- Theme integration: Spacing and styling verification
The test organization is clear with logical groupings and the helper method pattern is well-implemented.
lib/src/theme/components/breadcrumb.dart (1)
1-149: LGTM! Well-implemented breadcrumb theme class.This theme class provides comprehensive theming support for breadcrumb components with:
- Complete property set: separator, ellipsis, spacing, text styles, alignment
- Proper documentation: Clear doc comments for all properties
- Correct patterns: Follows established theme class patterns with merge flag
- Complete implementation: copyWith, mergeWith, lerp, equality, and hashCode
- Type safety: Immutable class with proper nullable types
The implementation is consistent with other theme classes in the codebase and provides all necessary customization options.
lib/src/components/breadcrumb.dart (5)
1-4: LGTM: Clean import structureThe imports are well-organized and appropriate for a themed UI component.
70-79: LGTM: Solid separation logic implementationThe logic correctly handles spacing and separators between breadcrumb items without adding trailing separators.
118-131: LGTM: Proper text styling with theme integrationThe conditional styling based on
isCurrentPageis well-implemented with appropriate theme fallbacks.
175-197: LGTM: Well-implemented interactive behaviorThe hover state management and conditional styling based on the presence of
onPressedcallback provides good user experience.
220-224: LGTM: Clean wrapper implementation
ShadBreadcrumbPageappropriately delegates toShadBreadcrumbItemwith the current page flag set.
| final effectiveSeparator = separator ?? | ||
| breadcrumbTheme.separator ?? | ||
| Icon( | ||
| LucideIcons.chevronRight, | ||
| size: 14, | ||
| color: theme.colorScheme.mutedForeground, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize default separator icon size
The default chevron icon size here is 14, but ShadBreadcrumbSeparator uses size 16 (line 254). This inconsistency could lead to visual misalignment when mixing different separator approaches.
Consider standardizing to a single size:
Icon(
LucideIcons.chevronRight,
- size: 14,
+ size: 16,
color: theme.colorScheme.mutedForeground,
);🤖 Prompt for AI Agents
In lib/src/components/breadcrumb.dart around lines 52 to 58, the default chevron
icon size is set to 14, which is inconsistent with the size 16 used in
ShadBreadcrumbSeparator at line 254. To fix this, update the icon size here to
16 to standardize the separator icon size across the codebase and ensure visual
alignment.
lib/src/components/breadcrumb.dart
Outdated
| return child ?? | ||
| breadcrumbTheme.separator ?? | ||
| Icon( | ||
| LucideIcons.chevronRight, | ||
| size: 16, | ||
| color: theme.colorScheme.mutedForeground, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address code duplication and size inconsistency
This separator logic duplicates the fallback chain from ShadBreadcrumb (lines 52-58) and uses a different icon size (16 vs 14), creating inconsistency.
Consider extracting the default separator logic to a shared method:
+ static Widget _buildDefaultSeparator(ShadThemeData theme) {
+ return Icon(
+ LucideIcons.chevronRight,
+ size: 16,
+ color: theme.colorScheme.mutedForeground,
+ );
+ }
+
@override
Widget build(BuildContext context) {
final theme = ShadTheme.of(context);
final breadcrumbTheme = theme.breadcrumbTheme;
return child ??
breadcrumbTheme.separator ??
- Icon(
- LucideIcons.chevronRight,
- size: 16,
- color: theme.colorScheme.mutedForeground,
- );
+ _buildDefaultSeparator(theme);📝 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.
| return child ?? | |
| breadcrumbTheme.separator ?? | |
| Icon( | |
| LucideIcons.chevronRight, | |
| size: 16, | |
| color: theme.colorScheme.mutedForeground, | |
| ); | |
| } | |
| static Widget _buildDefaultSeparator(ShadThemeData theme) { | |
| return Icon( | |
| LucideIcons.chevronRight, | |
| size: 16, | |
| color: theme.colorScheme.mutedForeground, | |
| ); | |
| } | |
| @override | |
| Widget build(BuildContext context) { | |
| final theme = ShadTheme.of(context); | |
| final breadcrumbTheme = theme.breadcrumbTheme; | |
| return child ?? | |
| breadcrumbTheme.separator ?? | |
| _buildDefaultSeparator(theme); | |
| } |
🤖 Prompt for AI Agents
In lib/src/components/breadcrumb.dart around lines 250 to 257, the separator
logic duplicates the fallback chain found in ShadBreadcrumb (lines 52-58) and
uses a different icon size, causing inconsistency. Extract the default separator
logic into a shared method that returns the separator widget with a consistent
icon and size (14), then replace the duplicated code with calls to this shared
method to ensure uniformity and reduce duplication.
Updates the calendar week number test to accurately check for week number rendering. It now verifies the existence of at least one week number text to avoid potential false negatives due to day numbers coinciding with week numbers.
|
Fixed the failing calendar test, but the flutter test CI needs to rerun. |
If you make a new commit, the CI runs again |
Adds the ability to customize the size of the breadcrumb ellipsis through the theme. This allows developers to adjust the visual appearance of the ellipsis to better fit their design requirements.
Indicates that the breadcrumb component has been fully implemented by updating the checkbox in the README.
|
@nank1ro I can't seem to be able to reproduce the calendar test that keeps failing. Locally, it always runs fine (after my fix). Can you help out on that part? |
There was a problem hiding this comment.
Does the component can render a dropdown item like the official one shown here https://ui.shadcn.com/docs/components/breadcrumb#dropdown?
no worries about the failing test, I'll fix it on main |
Replaces custom hover/focus styling on breadcrumb links with ShadButton's built-in styles. Reduces default breadcrumb spacing to improve visual balance.
Implements a new breadcrumb dropdown component, allowing to collapse breadcrumb items into a dropdown menu. This enhances the breadcrumb component to handle scenarios with a large number of navigation levels. The dropdown is implemented using a popover and is triggered by an ellipsis icon by default but can be customized.
Now it does |
|
I'm adjusting the padding, which is now off for the last element |
Allows customization of padding for breadcrumb items and page items (current page) via the theme. This change introduces `itemPadding` and `pagePadding` properties to the `ShadBreadcrumbTheme`, enabling developers to control the spacing around breadcrumb elements. Padding is only applied if the breadcrumb item is not a link or a button, as those components already have their own built-in padding.
|
Way better ! Any other request @nank1ro or are we ready to merge? |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
lib/src/components/breadcrumb.dart (2)
55-61: Icon size inconsistency still exists.The issue identified in previous reviews persists: this default separator uses size 14, while
ShadBreadcrumbSeparatoruses size 16 (lines 232-236). This creates visual inconsistency when mixing different separator approaches.Apply this fix to standardize the icon size:
Icon( LucideIcons.chevronRight, - size: 14, + size: 16, color: theme.colorScheme.mutedForeground, );
230-237: Code duplication and size inconsistency persists.The separator logic still duplicates the fallback chain from
ShadBreadcrumb(lines 55-61) and uses a different icon size (16 vs 14), as identified in previous reviews.Consider extracting the default separator logic to a shared method to eliminate duplication and ensure consistency:
class ShadBreadcrumb extends StatelessWidget { + static Widget _buildDefaultSeparator(ShadThemeData theme) { + return Icon( + LucideIcons.chevronRight, + size: 16, + color: theme.colorScheme.mutedForeground, + ); + } + @override Widget build(BuildContext context) { // ... existing code ... final effectiveSeparator = separator ?? breadcrumbTheme.separator ?? - Icon( - LucideIcons.chevronRight, - size: 14, - color: theme.colorScheme.mutedForeground, - ); + _buildDefaultSeparator(theme); // ... rest of method ... } } class ShadBreadcrumbSeparator extends StatelessWidget { @override Widget build(BuildContext context) { final theme = ShadTheme.of(context); final breadcrumbTheme = theme.breadcrumbTheme; return child ?? breadcrumbTheme.separator ?? - Icon( - LucideIcons.chevronRight, - size: 16, - color: theme.colorScheme.mutedForeground, - ); + ShadBreadcrumb._buildDefaultSeparator(theme); } }
🧹 Nitpick comments (4)
test/src/components/breadcrumb_test.dart (3)
246-250: Consider using theme-based size validation.The hardcoded size validation (36x36) in the test might break if the theme's
ellipsisSizeis changed. Consider validating against the theme's ellipsis size or making the expected size configurable.final sizedBoxFinder = find.descendant( of: ellipsisFinder, matching: find.byWidgetPredicate((widget) => widget is SizedBox && - widget.width == 36 && - widget.height == 36), + widget.width != null && + widget.height != null), );
540-546: Improve dropdown close test reliability.The comment indicates uncertainty about popover behavior in tests. Consider using a more deterministic approach to test dropdown closing behavior, such as testing the controller state directly.
// Since popovers might not close on tap outside in tests, let's just verify // that the dropdown can be closed by tapping the trigger again await tester.tap(find.byIcon(LucideIcons.ellipsis)); await tester.pumpAndSettle(); - // Verify dropdown is closed - expect(find.text('Item 1'), findsNothing); + // Verify dropdown controller state + expect(_controller.isOpen, false);Note: This would require accessing the controller, which might need test-specific methods.
692-692: Hardcoded padding values may cause brittleness.The test validates specific padding values (horizontal: 16, vertical: 8) that are hardcoded. If the default theme padding changes, this test will fail. Consider making this more flexible or validating against theme values.
final padding = tester.widget<Padding>(paddingFinder); - expect(padding.padding, const EdgeInsets.symmetric(horizontal: 16, vertical: 8)); + expect(padding.padding, isA<EdgeInsets>()); + expect((padding.padding as EdgeInsets).horizontal, greaterThan(0)); + expect((padding.padding as EdgeInsets).vertical, greaterThan(0));lib/src/components/breadcrumb.dart (1)
263-276: Consider making ellipsis dimensions themeable.The hardcoded 36x36 dimensions should use theme values when available, as mentioned in previous review comments. The current implementation addresses this partially but could be more consistent.
return SizedBox( - width: 36, - height: 36, + width: breadcrumbTheme.ellipsisSize?.width ?? 36, + height: breadcrumbTheme.ellipsisSize?.height ?? 36, child: Center( child: child ?? breadcrumbTheme.ellipsis ?? Icon( LucideIcons.ellipsis, size: 14, color: theme.colorScheme.mutedForeground, ), ), );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/src/components/breadcrumb.dart(1 hunks)lib/src/theme/components/breadcrumb.dart(1 hunks)lib/src/theme/themes/default_theme_no_secondary_border_variant.dart(2 hunks)lib/src/theme/themes/default_theme_variant.dart(2 hunks)test/src/components/breadcrumb_test.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/src/theme/themes/default_theme_variant.dart
- lib/src/theme/themes/default_theme_no_secondary_border_variant.dart
🧰 Additional context used
🧠 Learnings (2)
lib/src/theme/components/breadcrumb.dart (1)
Learnt from: nank1ro
PR: nank1ro/flutter-shadcn-ui#352
File: docs/src/content/docs/index.md:129-135
Timestamp: 2025-05-13T09:57:53.516Z
Learning: The shadcn_ui package uses `WidgetStatePropertyAll` in theme configurations (such as `ScrollbarThemeData`) instead of Flutter's native `MaterialStatePropertyAll`. This is intentional and correct according to the package maintainer.
lib/src/components/breadcrumb.dart (1)
Learnt from: nank1ro
PR: nank1ro/flutter-shadcn-ui#352
File: docs/src/content/docs/index.md:129-135
Timestamp: 2025-05-13T09:57:53.516Z
Learning: The shadcn_ui package uses `WidgetStatePropertyAll` in theme configurations (such as `ScrollbarThemeData`) instead of Flutter's native `MaterialStatePropertyAll`. This is intentional and correct according to the package maintainer.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Flutter test
🔇 Additional comments (9)
test/src/components/breadcrumb_test.dart (1)
10-18: Excellent test helper method design.The
createTestWidgethelper method provides a clean way to wrap test widgets in the required ShadApp and Scaffold context, ensuring consistent test setup across all test cases.lib/src/theme/components/breadcrumb.dart (4)
5-26: Excellent theme class documentation and structure.The class is well-documented with clear property descriptions and follows Flutter's immutable theme pattern correctly. The comprehensive set of customizable properties covers all necessary styling aspects for breadcrumb components.
110-130: Correct mergeWith implementation.The merge logic properly respects the
mergeflag and provides a clean way to combine theme instances while preserving the intended override behavior.
132-172: Robust lerp implementation with proper interpolation.The linear interpolation method correctly handles different property types using appropriate lerp methods (Size.lerp, TextStyle.lerp, EdgeInsets.lerp) and uses conditional logic for non-interpolatable properties.
174-214: Complete equality and hashCode implementation.The equality operator and hashCode implementation correctly include all properties, ensuring proper comparison behavior and collection usage.
lib/src/components/breadcrumb.dart (4)
79-84: Excellent solution for spacing implementation.The updated implementation using
separatedBywithPaddingelegantly addresses the previous concerns about spacing customization. This approach eliminates code duplication while providing proper spacing control.
171-178: Good use of ShadButton for consistency.Using
ShadButton.ghostprovides better consistency with the overall design system and offers more customization options compared to a custom implementation.
305-312: Proper state management and resource cleanup.The dropdown state management correctly uses a popover controller and properly disposes of resources in the dispose method, following Flutter best practices.
339-363: Well-structured popover implementation.The dropdown popover implementation is clean and follows the shadcn/ui design patterns. The use of
IntrinsicWidthand proper spacing creates a good user experience.
|
@Cobolt77 can you fix the conflicts so I can try the component locally? I have already solved on main the calendar test, use that version. |
|
@nank1ro Here you go! |
There was a problem hiding this comment.
There are some design problems that do not respect the original.
Original
vs yours
The spaces do not match, the font is incorrect, the text color should change according to the hover, the background should not be seen when hovering but should only be a link.
To give you more details:
- ShadButton.ghost should not be used, use ShadButton.raw with a default link variant.
- Double check the default styles of the original shadcn component (you can find it here)
- ShadBreadcrumbPage seems useless, it's all already provided by ShadBreadcrumbItem. (I don't see the need to provide a new Widget just to mock a boolean property)
- ShadBreadcrumbDropdown is useless too, the user can just wrap it from the outside with a Popover and get all the available customizations. (The design of the popover doesn't respect the original too)
- Never do inferences like this one
if (child is! ShadButton && child is! ShadBreadcrumbLink), they can produce runtime differences which are hard to track. What if child is a Padding that wraps ShadButton? It sounds like a workaround. - The component example is not linked in the
main.appand doesn't use the BaseScaffold like other examples. (See Contributing)
Adds a Breadcrumb example page to the example application showcasing various features of the breadcrumb component. This includes: - Basic breadcrumb display - Breadcrumb with clickable links - Breadcrumb with ellipsis to handle overflow - Custom separator styles Removes dropdown functionality from the base component, simplifying the API.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
test/src/components/breadcrumb_test_new.dart (1)
1-288: Remove duplicate test fileThis test file is nearly identical to
breadcrumb_test.dartwith only a minor difference in line 217 (checking forMouseRegionvsShadBreadcrumbLink). Having duplicate test files creates maintenance overhead and confusion.Consider consolidating both test approaches into a single test file:
-expect(find.byType(ShadBreadcrumbLink), findsOneWidget); +expect(find.byType(MouseRegion), findsOneWidget); +expect(find.byType(ShadBreadcrumbLink), findsOneWidget);Then remove this duplicate file entirely.
🧹 Nitpick comments (1)
lib/src/components/breadcrumb.dart (1)
140-158: Consider using ShadButton for better consistencyBased on past review discussions, using ShadButton would provide better customization options and consistency with the rest of the UI library. The current implementation manually handles hover states and styling.
Consider refactoring to use ShadButton with appropriate theming:
- return MouseRegion( - cursor: widget.onPressed != null - ? SystemMouseCursors.click - : SystemMouseCursors.basic, - onEnter: (_) => setState(() => _isHovered = true), - onExit: (_) => setState(() => _isHovered = false), - child: GestureDetector( - onTap: widget.onPressed, - child: DefaultTextStyle( - style: theme.textTheme.small.copyWith( - color: _isHovered - ? theme.colorScheme.foreground - : theme.colorScheme.mutedForeground, - fontWeight: FontWeight.normal, - ), - child: widget.child, - ), - ), - ); + return ShadButton.ghost( + onPressed: widget.onPressed, + child: widget.child, + // Apply appropriate styling through theme + );This would leverage the existing button infrastructure and provide better customization options.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
example/lib/main.dart(2 hunks)example/lib/pages/breadcrumb_example.dart(1 hunks)lib/src/components/breadcrumb.dart(1 hunks)lib/src/theme/components/breadcrumb.dart(1 hunks)playground/lib/pages/breadcrumbs.dart(1 hunks)test/src/components/breadcrumb_test.dart(1 hunks)test/src/components/breadcrumb_test_new.dart(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- example/lib/main.dart
🚧 Files skipped from review as they are similar to previous changes (3)
- example/lib/pages/breadcrumb_example.dart
- playground/lib/pages/breadcrumbs.dart
- lib/src/theme/components/breadcrumb.dart
🧰 Additional context used
🧠 Learnings (1)
lib/src/components/breadcrumb.dart (1)
Learnt from: nank1ro
PR: nank1ro/flutter-shadcn-ui#352
File: docs/src/content/docs/index.md:129-135
Timestamp: 2025-05-13T09:57:53.516Z
Learning: The shadcn_ui package uses `WidgetStatePropertyAll` in theme configurations (such as `ScrollbarThemeData`) instead of Flutter's native `MaterialStatePropertyAll`. This is intentional and correct according to the package maintainer.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Flutter test
🔇 Additional comments (3)
test/src/components/breadcrumb_test.dart (1)
1-289: Excellent test coverage for breadcrumb components!The test suite is comprehensive and well-structured, covering all major functionality including basic rendering, interactions, custom separators, ellipsis handling, and text direction. The tests follow Flutter testing best practices with proper async pumping and descriptive test names.
lib/src/components/breadcrumb.dart (2)
53-59: LGTM - Separator icon size standardizedThe separator icon size has been correctly standardized to 14, resolving the previous inconsistency issue mentioned in past reviews.
1-260: Overall implementation looks solidThe breadcrumb component suite is well-implemented with:
- Comprehensive widget set covering all breadcrumb use cases
- Proper theming integration
- Good documentation with dartdoc comments
- Clean separation of concerns between components
- Appropriate use of Flutter widgets and patterns
The component follows Flutter best practices and provides a flexible, customizable breadcrumb navigation solution.
| return SizedBox( | ||
| width: 36, | ||
| height: 36, | ||
| child: Center( | ||
| child: child ?? | ||
| Icon( | ||
| LucideIcons.ellipsis, | ||
| size: 16, | ||
| color: theme.colorScheme.mutedForeground, | ||
| ), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize icon size and make dimensions themeable
The ellipsis icon uses size 16 while separators use size 14, creating visual inconsistency. Additionally, the SizedBox dimensions are hardcoded.
Apply this diff to align with the separator icon size and make dimensions themeable:
+ @override
+ Widget build(BuildContext context) {
+ final theme = ShadTheme.of(context);
+ final breadcrumbTheme = theme.breadcrumbTheme;
+
+ final effectiveSize = breadcrumbTheme.ellipsisSize ?? 36.0;
+
return SizedBox(
- width: 36,
- height: 36,
+ width: effectiveSize,
+ height: effectiveSize,
child: Center(
child: child ??
Icon(
LucideIcons.ellipsis,
- size: 16,
+ size: 14,
color: theme.colorScheme.mutedForeground,
),
),
);This assumes the theme includes an ellipsisSize property. If not available, consider adding it to the breadcrumb theme.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/src/components/breadcrumb.dart around lines 246 to 257, the ellipsis icon
size is hardcoded to 16 while separators use size 14, causing inconsistency, and
the SizedBox dimensions are fixed. To fix this, change the icon size to match
the separator size (14) and replace the hardcoded width and height of the
SizedBox with themeable properties, such as using a new ellipsisSize property
from the breadcrumb theme. If the theme does not have this property, add it to
the breadcrumb theme for consistency and configurability.
Increases the horizontal padding around the breadcrumb separator to improve visual clarity.
|
The only remaining issue would be the font weight: |
|
@nank1ro Do you have any idea why the same font, with the same weight applied to it, doesn't render the same? |
Flutter has many issues regarding font rendering, especially on web, which platform are you trying it on? |
|
@nank1ro I was trying it out on the web. Any other things to fix ? |
|
I'll review it as soon as I have some free time. |
Hi @Cobolt77, would like to check your PR and merge it if it's all fine, can I have your answer here? |
|
Hi @nank1ro ! Sorry for the delay. Using the ShadCN button, I couldn't get the layout and spacing right, even after hours of trying. This is why I switched to Gesture detector. |
Adding the ShadBreadcrumb implementation mentionned in this issue #236
Pre-launch Checklist
///).If you need help, consider asking for advice on Discord.
Summary by CodeRabbit
New Features
Documentation
Tests