feat: "Naked" - A Behavior-First UI Component Library for Flutter #579
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
WalkthroughThis update introduces the new "naked" package, a comprehensive Flutter library providing unstyled, behavior-first UI components that separate interaction logic from presentation. The package includes core widgets such as buttons, checkboxes, sliders, tabs, accordions, menus, selects, tooltips, radio buttons, dialogs, and text fields, each designed to be fully customizable and accessible. The release also adds extensive example implementations, integration and widget tests, configuration files, and detailed documentation covering development philosophy, usage patterns, state management, and accessibility guidelines. Utility modules for focus management, overlay portals, and adaptive positioning are provided, supporting advanced interaction and layout needs. The package is structured for ease of extension and integration into custom design systems. Changes
Sequence Diagram(s)sequenceDiagram
participant App as ShowcaseApp
participant User
participant Widget as NakedComponent
participant State as StateManager
User->>App: Selects component from navigation
App->>Widget: Renders selected NakedComponent example
Widget->>State: Emits state change via callback (hover, press, focus, etc.)
State->>Widget: Updates UI based on state
Widget-->>App: Triggers navigation or feedback as needed
User->>Widget: Interacts (click, hover, keyboard)
Widget->>State: Notifies of interaction state
sequenceDiagram
participant NakedButton
participant ConsumerWidget
participant User
User->>NakedButton: Hover/Press/Focus
NakedButton->>ConsumerWidget: onHoverState/onPressedState/onFocusState callbacks
ConsumerWidget->>NakedButton: Updates visual styling based on state
Suggested labels
Poem
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:
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: 11
♻️ Duplicate comments (2)
packages/naked/README.md (1)
133-134: Repeated documentation link with the same potential issue.Similar to the previous comment, verify that this link will be accessible to users of the published package.
packages/naked/example/lib/examples/naked_checkbox_example.dart (1)
595-676: Repeated comment about code duplication.The comment on line 596 acknowledges the code duplication issue: "Re-use the visual builder from BasicCheckboxStates (or move it to a common place)". This is a good recognition of the issue, but it would be better to actually implement the suggested refactoring.
🧹 Nitpick comments (57)
packages/naked/analysis_options.yaml (1)
28-36: Clear Custom Lint Documentation
The inline comments detailing property naming conventions (e.g., usingisfor booleans andonfor callbacks) are very helpful. Ensure these comments are kept up to date with any future modifications in coding standards.packages/naked/lib/src/utilities/utilities.dart (1)
7-10: Address Duplicate Export Comment
There is a commented-out export for'naked_positioning.dart'on line 7 even though the file is actively exported on line 10. To avoid confusion, consider removing the commented-out line if it is no longer needed.packages/naked/example/pubspec.yaml (1)
30-50: Well-organized dependenciesThe dependencies are well-structured with clear comments explaining their purpose. The path dependency to the root package is correctly configured.
Remove trailing whitespace on lines 41 and 44:
cupertino_icons: ^1.0.6 - + # URL strategy for web url_strategy: ^0.2.0 - + # Routing🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 41-41: trailing spaces
(trailing-spaces)
[error] 44-44: trailing spaces
(trailing-spaces)
packages/naked/example/README.md (1)
18-38: Consider enhancing test instructions for clarityThe testing instructions are comprehensive, but could benefit from small improvements:
- Line 24: "Ensure the macOS platform is supported"
- Line 36: "Test results will be displayed on the terminal" (instead of "in")
These minor adjustments would improve readability and correctness.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~24-~24: You might be missing the article “the” here.
Context: ...macOS, follow these steps: 1. Ensure macOS platform is supported: ```bash ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~36-~36: The preposition “on” seems more likely in this position than the preposition “in”.
Context: ...ing - Test results will be displayed in the terminal - Any failures will be ...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_ON)
packages/naked/pubspec.yaml (3)
1-5: Remove commented instructions in versioning and repository URLThe package metadata contains comments that appear to be development notes:
-version: 0.0.1-dev.0 # Or keep 0.0.1 if that's correct -repository: https://github.com/leofarias/naked # Restore original repo URL +version: 0.0.1-dev.0 +repository: https://github.com/leofarias/nakedThese comments should be removed before publishing.
6-9: Remove commented instructions in environment constraintsThe environment constraints section contains comments that appear to be development notes:
- sdk: '>=3.0.0 <4.0.0' # Restore original SDK constraint - flutter: '>=3.10.0' # Restore original Flutter constraint + sdk: '>=3.0.0 <4.0.0' + flutter: '>=3.10.0'These comments should be removed before publishing.
10-18: Remove commented instruction in dev_dependenciesThe dev_dependencies section contains a comment that appears to be a development note:
- flutter_lints: ^2.0.0 # Restore original lints version + flutter_lints: ^2.0.0This comment should be removed before publishing.
packages/naked/example/lib/url_strategy.dart (1)
11-36: Consider distinguishing web and mobile initialization pathsThe UrlStrategy.initialize() method calls getUrlStrategy().initializeInstance() in both the web and mobile branches (lines 30 and 33) with identical code. While the actual implementation likely differs in the platform-specific files, consider adding comments to clarify what platform-specific initialization is happening in each case, or restructure to make the distinction clearer.
packages/naked/test/naked_avatar_test.dart (1)
125-164: Loading and error callbacks test could be improved.While the test verifies that loading and error callbacks can be provided, it doesn't actually test their functionality in response to real loading/error events.
Consider enhancing this test to actually trigger loading and error states and verify the callbacks are invoked with the correct values, perhaps by using a mocked image provider that can simulate these states.
packages/naked/test/naked_accordion_focus_test.dart (2)
73-115: Focus navigation test is somewhat brittle.While the test correctly verifies focus movement between accordion triggers, the approach of finding Text widgets that are descendants of Focus widgets and then manually iterating through them is somewhat brittle.
Consider using a more robust approach for verifying focus state, such as using the FocusNode directly or adding key values to each trigger for more precise targeting.
- final focusableTexts = - find.descendant(of: find.byType(Focus), matching: find.byType(Text)); - - bool foundTrigger2 = false; - tester.widgetList(focusableTexts).forEach((widget) { - if (widget is Text && widget.data == 'Trigger 2') { - foundTrigger2 = true; - } - }); - expect(foundTrigger2, isTrue); + final trigger2 = find.text('Trigger 2'); + expect( + (find.ancestor(of: trigger2, matching: find.byType(Focus)) + .evaluate() + .first + .widget as Focus) + .focusNode + .hasFocus, + isTrue + );
180-203: Weak assertion in callback test.The test for the onFocusItem callback uses a weak assertion that would pass even if the callback is never called.
Consider using a more definitive assertion to ensure the callback is actually triggered:
- // Since we can't reliably check the exact callback in the test environment, - // we'll just verify that the callback functionality exists - expect(callbackCount, greaterThanOrEqualTo(0)); + // Simulate actions that should trigger the callback + await tester.sendKeyEvent(LogicalKeyboardKey.tab); + await tester.pump(); + + // Check that the callback was actually called at least once + expect(callbackCount, greaterThan(0));packages/naked/example/integration_test/app_navigation_test.dart (2)
55-67: Consider providing more specific reasons for skipped tests.The test skips certain examples but provides limited information about why they're being skipped.
Add more specific comments explaining exactly why the Avatar and Focus examples are problematic in integration tests to help future developers understand these limitations.
// Skip Avatar example (index 1) as it makes network calls if (i == 1) continue; // Skip examples causing layout issues in tests if (destination.label.contains('Focus')) { - debugPrint('Skipping focus example: ${destination.label}'); + debugPrint('Skipping focus example: ${destination.label} because focus testing requires special handling in integration tests'); continue; }
85-87: Arbitrary delay could cause test flakiness.Adding a fixed delay duration could make tests flaky on slower devices or in CI environments.
Consider using
pumpAndSettlewith a timeout parameter instead of a fixed delay to make the test more robust across different environments:- // Add extra pumps to ensure the UI has fully updated - await tester.pump(const Duration(milliseconds: 500)); - await tester.pumpAndSettle(); + // Wait for UI to fully settle with a reasonable timeout + await tester.pumpAndSettle(const Duration(seconds: 2));packages/naked/lib/src/naked_radio_button.dart (3)
175-197: Consider adding a toggle behavior for selected items.When tapping a radio button that is already selected, nothing happens. Users might expect some feedback, even if the value doesn't change.
Consider providing feedback (like haptic or visual) when tapping an already selected radio button to improve user experience, even if the value doesn't change:
// If group is already set to this value, do nothing if (group.value == widget.value) { + // Still provide haptic feedback if enabled, to indicate the tap was recognized + if (widget.enableHapticFeedback) { + HapticFeedback.selectionClick(); + } return; }
214-222: Duplicate error handling logic.There's duplicate error-throwing code in both the build method and the _handleTap method.
Consider extracting the error checking into a helper method to avoid duplication:
+ // Helper method to get radio group or throw error + NakedRadioGroupScope<T>? _getRadioGroupOrThrow() { + final group = NakedRadioGroupScope.of<T>(context); + if (group == null) { + throw FlutterError( + 'NakedRadioButton must be used within a NakedRadioGroup.\n' + 'No NakedRadioGroup ancestor could be found for a NakedRadioButton with value: ${widget.value}', + ); + } + return group; + } void _handleTap() { - final group = NakedRadioGroupScope.of<T>(context); - - if (group == null) { - throw FlutterError( - 'NakedRadioButton must be used within a NakedRadioGroup.\n' - 'No NakedRadioGroup ancestor could be found for a NakedRadioButton with value: ${widget.value}', - ); - } + final group = _getRadioGroupOrThrow(); // Rest of the method } @override Widget build(BuildContext context) { // Get the group this radio button belongs to - final group = NakedRadioGroupScope.of<T>(context); - - // If no group found, show error in debug mode - if (group == null) { - assert(() { - throw FlutterError( - 'NakedRadioButton must be used within a NakedRadioGroup.\n' - 'No NakedRadioGroup ancestor could be found for a NakedRadioButton with value: ${widget.value}', - ); - }()); - return widget.child; - } + final group = _getRadioGroupOrThrow(); // Rest of the method }
252-256: Key event handling always returnshandled.The onKeyEvent handler always returns KeyEventResult.handled, which might prevent other widgets from processing key events.
Consider returning KeyEventResult.skipRemainingHandlers when appropriate to allow event propagation:
onKeyEvent: (node, event) { if (isInteractive) { _handleKeyEvent(event); + return KeyEventResult.handled; } - return KeyEventResult.handled; + return KeyEventResult.skipRemainingHandlers; },packages/naked/lib/naked.dart (1)
13-13: Documentation path may not be accessible after package publication.The path
.context/plan/naked_component_development_guide.mdappears to be a repository-relative path. When this package is published, users may not have access to this file.Consider moving this documentation to a more accessible location like a public website or including it directly in the package's documentation.
packages/naked/example/lib/url_strategy_mobile.dart (1)
25-29: Redundant initialize method.The
initialize()static method just wraps the instance methodinitializeInstance(), which itself is a no-op. This adds unnecessary indirection.While this might be for API consistency with other platforms, consider documenting this explicitly or simplifying if the wrapper is not needed.
packages/naked/example/lib/examples/naked_positioning_example.dart (3)
148-163: Position update logic could be more robust.The
_updateButtonRectmethod is only called when the button is pressed, which means the rectangle position won't update if the screen size changes or rotates.Consider adding a layout listener or using a LayoutBuilder to update the button's position whenever the layout changes:
class _NakedPositioningExampleState extends State<NakedPositioningExample> { final _buttonKey = GlobalKey(); bool _showContent = false; Rect? _buttonRect; + LayoutCallback? _layoutCallback; // ... other variables + @override + void initState() { + super.initState(); + WidgetsBinding.instance.addPostFrameCallback((_) { + _updateButtonRect(); + }); + } + + @override + void didChangeDependencies() { + super.didChangeDependencies(); + _updateButtonRect(); + } // ... rest of the class }
165-208: Safety check for label access needed.In
_buildPositionedContent(), there's a potential for a null value when accessing_positionLabels[_selectedPosition]if for some reason the selected position is not in the map.Add a null-safe access:
- Text( - 'Positioned with ${_positionLabels[_selectedPosition]}', - style: const TextStyle( + Text( + 'Positioned with ${_positionLabels[_selectedPosition] ?? "Unknown position"}', + style: const TextStyle(
166-208: Consider handling off-screen positioning.The example doesn't handle cases where the positioned content might extend beyond the screen boundaries.
Consider adding boundary detection or illustrating how
NakedPositioninghandles these edge cases. This would make the example more comprehensive and practical.packages/naked/example/lib/url_strategy_web.dart (1)
20-32: JS interop implementation includes proper error handling.The updateUrl method properly handles errors that might occur during JS interop calls, but consider using a more specific error type instead of the generic catch-all.
try { // Access history from window object and call replaceState method js.context['history'].callMethod('replaceState', [null, '', url]); - } catch (e) { + } catch (e, stackTrace) { // Handle the error gracefully - debugPrint('Error updating URL: $e'); + debugPrint('Error updating URL: $e\n$stackTrace'); }packages/naked/example/lib/examples/naked_avatar_example.dart (1)
112-181: Consider refactoring the avatar group implementation.The avatar group implementation uses a Stack with fixed positioning, which works but might be less maintainable. Consider creating a reusable AvatarGroup component that handles the positioning and overlapping logic internally.
A dedicated component could handle varying group sizes more flexibly:
// Suggested implementation for a reusable component class AvatarGroup extends StatelessWidget { final List<Widget> avatars; final double avatarSize; final double overlap; const AvatarGroup({ Key? key, required this.avatars, required this.avatarSize, this.overlap = 16.0, }) : super(key: key); @override Widget build(BuildContext context) { final double groupWidth = avatarSize * avatars.length - (overlap * (avatars.length - 1)); return SizedBox( height: avatarSize, width: groupWidth, child: Stack( children: List.generate(avatars.length, (index) { final leftPosition = index * (avatarSize - overlap); return Positioned( left: leftPosition, top: 0, child: avatars[index], ); }), ), ); } }packages/naked/lib/src/naked_checkbox.dart (1)
209-213: Refine tri-state semantics.Currently,
Semantics(checked: isChecked, toggled: isIndeterminate)may lead to incorrect screen reader interpretation of indeterminate states. Consider using the result ofgetCurrentValue()forchecked(null when indeterminate) and omittingtoggledfor more accurate tri-state accessibility.- checked: isChecked, - toggled: isIndeterminate, + checked: getCurrentValue(),packages/naked/example/lib/examples/naked_menu_example.dart (1)
24-26: Remove or dynamically manage the_isSubmenuOpenfinal field.Declaring
_isSubmenuOpenasfinalin_NakedMenuExampleStateprevents the submenu from ever toggling open. If this is intentional, remove it; otherwise, allow it to change to reflect actual nested submenu usage.- final bool _isSubmenuOpen = false; + bool _isSubmenuOpen = false;packages/naked/lib/src/naked_avatar.dart (1)
223-245: Optionally allow user overrides of hover/press visuals.The
MouseRegionandGestureDetectorhandle hover and press states internally with callback triggers, but some consumers might want further customization (e.g., different pointer interactions or additional states). Exposing an optional builder or inheritance pattern can increase flexibility if needed.packages/naked/example/lib/examples/naked_select_example.dart (1)
34-54: Reduce duplication by extracting repeated state variables and logic into helper classes or mixins.Each select component declares multiple hover, press, and focus states in a very similar pattern. This repetition can become difficult to maintain. Extracting these into a reusable helper or a specific state management approach can streamline the code and reduce duplication.
packages/naked/lib/src/utilities/naked_focus_manager.dart (1)
123-142: Consider expanding keyboard event handling beyond Escape.Currently, only the Escape key triggers an action. If additional keys (e.g., Tab, Arrow keys, or Enter) might need special handling inside this manager, exposing a more comprehensive keyboard event callback or hooking into multiple relevant keys could improve extensibility.
packages/naked/lib/src/utilities/naked_portal.dart (1)
48-120: Check user experience for frequent updates.
WhendidUpdateWidgettriggers_updateOverlayEntry, it removes and re-inserts anOverlayEntry, which may cause noticeable flickering if the child is updated very frequently. Consider optimizing the update mechanism or adding logic to detect whether the child actually changed before re-inserting the overlay.packages/naked/example/lib/error_boundary.dart (1)
42-57: Consider re-throwing errors for logging or crash reporting.
Currently,_handleFlutterErrorupdates_errorin state and invokes_originalOnError. If you need crash reporting or external logging, consider adding logic to re-throw or report to third-party services (like Sentry) to avoid silently catching critical issues.packages/naked/naked_component_guide.md (2)
1-7: Include additional metadata or disclaimers if needed.While the front matter is clear, consider including versioning information or disclaimers about early-stage features if applicable. This can help set expectations for readers and communicate stability levels.
57-73: Emphasize error handling in callback-driven architecture.The guide covers the consumer-managed state approach thoroughly, but it may be useful to include discussion on how to handle potential errors or exceptions that occur when invoking callbacks (e.g., missing callbacks or throws). Adding a short subsection will improve developer readiness for edge cases.
packages/naked/lib/src/naked_tabs.dart (1)
189-193: Confirm the necessity of the_registerTabFocusNodeno-op.This method is maintained for backward compatibility but is currently a no-op. If there is no scenario still calling it, consider removing it or marking it deprecated to reduce confusion.
packages/naked/example/lib/examples/naked_button_example.dart (2)
66-75: Potential naming conflict with Flutter's built-inToggleButtons.Although it’s a nice example, there’s a built-in widget in Flutter named
ToggleButtons. Consider renaming this custom widget or adding a naming prefix to avoid confusion in your codebase.
338-403: Check layering for performance concerns with complex animations.Continuous layering (stack + positioned + animated container) can be more CPU-intensive on lower-end devices. It's likely acceptable, but keep an eye on performance if you plan to scale animations or nesting further.
packages/naked/README.md (1)
18-21: Consider specifying Flutter SDK version requirements.While the installation instructions include the version of the package, it would be helpful to specify the minimum required Flutter SDK version for compatibility purposes.
dependencies: naked: ^0.0.1-dev.0 + +# Requires Flutter SDK 2.10.0 or higherpackages/naked/example/lib/main.dart (4)
3-23: Consider organizing imports and clarifying commented code.The import section contains several commented-out import statements mixed with active ones, which can be confusing. It would be clearer to either:
- Remove commented imports if they're not needed
- Group commented imports separately with a clearer explanation of why they're commented
Additionally, line 23's comment about conditionally importing dart:js seems misplaced since it's followed by importing url_strategy.dart instead.
// Uncomment the accordion example import import 'examples/naked_accordion_example.dart'; - // import 'examples/naked_accordion_focus_example.dart'; import 'examples/naked_avatar_example.dart'; import 'examples/naked_button_example.dart'; import 'examples/naked_checkbox_example.dart'; - // import 'examples/naked_checkbox_focus_example.dart'; import 'examples/naked_menu_example.dart'; - // Assuming naked_positioning_example.dart is structured similarly - // import 'examples/naked_positioning_example.dart'; import 'examples/naked_radio_example.dart'; - // import 'examples/naked_radio_focus_example.dart'; import 'examples/naked_select_example.dart'; - // import 'examples/naked_select_focus_example.dart'; - // Assuming naked_select_enhanced_example.dart exists and is a widget - // import 'examples/naked_select_enhanced_example.dart'; import 'examples/naked_slider_example.dart'; import 'examples/naked_tabs_example.dart'; - // import 'examples/naked_tabs_focus_example.dart'; - // Conditionally import dart:js import 'url_strategy.dart'; + // Import active examples + import 'examples/naked_accordion_example.dart'; + import 'examples/naked_avatar_example.dart'; + import 'examples/naked_button_example.dart'; + import 'examples/naked_checkbox_example.dart'; + import 'examples/naked_menu_example.dart'; + import 'examples/naked_radio_example.dart'; + import 'examples/naked_select_example.dart'; + import 'examples/naked_slider_example.dart'; + import 'examples/naked_tabs_example.dart'; + + // URL strategy for web support + import 'url_strategy.dart'; + + // Commented examples (to be implemented) + // import 'examples/naked_accordion_focus_example.dart'; + // import 'examples/naked_checkbox_focus_example.dart'; + // import 'examples/naked_positioning_example.dart'; + // import 'examples/naked_radio_focus_example.dart'; + // import 'examples/naked_select_focus_example.dart'; + // import 'examples/naked_select_enhanced_example.dart'; + // import 'examples/naked_tabs_focus_example.dart';
76-88: Consider adding error handling for unrecognized routes.While the code defaults to index 0 if a component name is not found, it might be more user-friendly to:
- Display an error message or redirect to a 404 page for completely invalid routes
- Log the unrecognized route for debugging purposes
static int _getDestinationIndex(String componentName) { final normalizedName = componentName.toLowerCase().replaceAll('-', ''); for (int i = 0; i < destinations.length; i++) { final destName = destinations[i].label.toLowerCase().replaceAll(' ', ''); if (destName == normalizedName) { return i; } } // Default to 0 if component not found + debugPrint('Warning: Unknown component "$componentName", defaulting to first component'); return 0; }
113-141: Maintain consistency in commented code.Lines 130-136 use a multi-line comment for a commented-out
NavDestination, while the following commented-out destinations use single-line comments. For consistency and easier uncomment/comment operations, consider using the same comment style throughout.// Focus Examples (Use Icons.keyboard) // Commented out because the accordion focus example was deleted -/* -NavDestination( - label: 'Accordion Focus', - icon: Icons.keyboard, - widget: const NakedAccordionFocusExample()), -*/ +// NavDestination( +// label: 'Accordion Focus', +// icon: Icons.keyboard, +// widget: const NakedAccordionFocusExample()),
238-245: Consider adding keyboard navigation support.The navigation rail could benefit from explicit keyboard navigation support for accessibility. Consider adding keyboard shortcuts or arrow key navigation between the destinations.
NavigationRail( selectedIndex: _selectedIndex, extended: _isExtended, minExtendedWidth: 180, // Adjust width when extended onDestinationSelected: (int index) { // Use a post-frame callback to update the state // This prevents setState during layout/paint issues WidgetsBinding.instance.addPostFrameCallback((_) { if (mounted) { setState(() { _selectedIndex = index; // Update URL when tab changes _updateUrlForDestination(index); }); } }); }, + // Add methods to handle keyboard navigation + // This would require custom focus handling in a real implementationpackages/naked/example/lib/examples/naked_checkbox_example.dart (2)
59-63: Prefer a constant aspect ratio for more predictable layouts.The dynamic calculation of the aspect ratio based on screen width could lead to unpredictable layouts on edge cases. Consider using a fixed aspect ratio that works well across device sizes.
- childAspectRatio: (screenWidth / crossAxisCount) / - (crossAxisCount == 1 ? 120 : 150), + // Fixed aspect ratios for more predictable layouts + childAspectRatio: crossAxisCount == 1 ? 2.5 : 2.0,
144-146: Consider using controlled constants for state tracking variables.The state tracking variables for visual feedback are initialized with a comment indicating they're optional. If they're important for debugging/learning, consider making that more explicit in the code structure.
- // State trackers for visual feedback (optional, but good for debugging/learning) - bool _isHovered = false; - bool _isPressed = false; - bool _isFocused = false; + // State trackers for visual feedback - used to demonstrate state changes + final bool _isHovered = false; + final bool _isPressed = false; + final bool _isFocused = false;packages/naked/example/lib/examples/naked_tabs_example.dart (6)
118-129: Consider using an enum for tab IDs instead of strings.Using string literals for tab IDs can be error-prone. Consider using an enum to represent tab IDs for type safety and auto-completion benefits.
+enum BasicTabId { account, password, settings } + +extension BasicTabIdExtension on BasicTabId { + String get value { + return toString().split('.').last; + } +} class _BasicTabsState extends State<_BasicTabs> { - String _activeTab = 'account'; + BasicTabId _activeTab = BasicTabId.account; final Map<String, bool> _hoverStates = {}; final Map<String, bool> _focusStates = {}; final Map<String, bool> _pressStates = {}; final List<Map<String, String>> _tabs = [ - {'id': 'account', 'label': 'Account'}, - {'id': 'password', 'label': 'Password'}, - {'id': 'settings', 'label': 'Settings'}, + {'id': BasicTabId.account.value, 'label': 'Account'}, + {'id': BasicTabId.password.value, 'label': 'Password'}, + {'id': BasicTabId.settings.value, 'label': 'Settings'}, ];
135-138: Use const constructor for ConstrainedBox for better performance.When the constraints are known at compile time, using a const constructor improves widget rebuilding performance by allowing Flutter to reuse the widget instance.
- constraints: const BoxConstraints(maxWidth: 400), + constraints: const BoxConstraints(maxWidth: 400),
154-155: Consider caching state values to reduce object creation.The state variables for hover, focus, and press are retrieved in multiple places potentially creating new unneeded boolean objects. Consider caching these values.
final String id = tab['id']!; final bool isActive = _activeTab == id; -final bool isHovered = _hoverStates[id] ?? false; -final bool isFocused = _focusStates[id] ?? false; -final bool isPressed = _pressStates[id] ?? false; +// Cache state values to avoid repeated map lookups +final bool isHovered = _hoverStates[id] ?? false; +final bool isFocused = _focusStates[id] ?? false; +final bool isPressed = _pressStates[id] ?? false;
537-542: State tracking maps could be final.The state tracking maps for hover, focus, and press states are mutable but don't need to be reassigned. Using
finalclarifies the intention that the maps themselves don't change, only their contents.- String _activeTab = 'general'; - final Map<String, bool> _hoverStates = {}; - final Map<String, bool> _focusStates = {}; - final Map<String, bool> _pressStates = {}; + String _activeTab = 'general'; + final Map<String, bool> _hoverStates = {}; + final Map<String, bool> _focusStates = {}; + final Map<String, bool> _pressStates = {};
748-752: Consider extracting color constants for better maintainability.The color literals are hardcoded throughout the file. Consider extracting these to constants or a theme class for easier maintenance and consistency.
+// At the top of the file or in a separate constants file +class TabColors { + static const activeBlue = Color(0xFF3B82F6); + static const borderGray = Color(0xFFE5E7EB); + static const lightGray = Color(0xFFF3F4F6); + // ...more color constants +} // Then in the code borderRadius: BorderRadius.circular(8), border: Border.all( color: isActive - ? const Color(0xFFBFDBFE) // border-blue-200 - : const Color(0xFFE5E7EB), // border-gray-200 + ? TabColors.activeBlue.withOpacity(0.5) // border-blue-200 + : TabColors.borderGray, // border-gray-200 ),
1031-1111: Consider using a more robust data model for products.The product data could be represented using a dedicated class rather than maps for better type safety and maintainability. This would also make it easier to add methods or computed properties.
class Product { final String id; final String name; final String price; final String category; final IconData icon; const Product({ required this.id, required this.name, required this.price, required this.category, required this.icon, }); } // Then initialize products as: final List<Product> allProducts = [ Product( id: 'p1', name: 'Smartphone', price: '\$499', category: 'electronics', icon: Icons.smartphone, ), // ... ];packages/naked/lib/src/naked_select.dart (1)
450-894: Optionally close the dropdown on outside taps
Currently, clicks outside the dropdown are absorbed by theGestureDetectorbut do not close the menu. This may lead to a suboptimal user experience when trying to dismiss the select.You can conditionally detect taps outside and close the menu:
child: GestureDetector( behavior: HitTestBehavior.opaque, onTap: () {}, // Prevent taps from reaching background + // Potential approach: + // onTap: () { + // if (selectWidget.onIsOpenChanged != null) { + // selectWidget.onIsOpenChanged!(false); + // } + // }, child: NakedPositioning( ...packages/naked/lib/src/naked_slider.dart (1)
255-504: Consider supporting alternative drag gestures
The slider currently handles horizontal and vertical drags. If custom or diagonal drags are needed, a specialized drag gesture recognizer might be beneficial.You could add an optional parameter or strategy pattern to recognize different drag behaviors for advanced usage scenarios without extending the default gestures further than necessary.
packages/naked/example/lib/examples/naked_accordion_example.dart (1)
1-600: Promote reusability by refactoring repeated patterns
Sections like_buildSection,_BasicAccordion, and others demonstrate repeated patterns (title, padding, styling). Refactoring common logic into smaller shared widgets or helper methods can reduce code duplication and enhance maintainability.packages/naked/lib/src/utilities/naked_positioning.dart (1)
365-429: Minor readability suggestion: consider smaller helper methods.While
_AdaptiveSizeMeasureis neat, the post-frame callbacks can become frequent in larger or highly dynamic layouts. Depending on usage frequency, consider subdividing or optimizing repeated measure calls.packages/naked/lib/src/naked_accordion.dart (1)
423-526: Smooth expansion animation inNakedAccordionContent.The
SizeTransitioncombined with aCurvedAnimationyields a pleasing reveal/collapse effect. Consider allowing a configurabledurationfor advanced customization.packages/naked/example/lib/examples/naked_radio_example.dart (4)
1141-1157: Implement ButtonStyleRadio instead of using Placeholder.The
ButtonStyleRadioclass currently uses aPlaceholderwidget, which renders a crossed rectangle. Since this is an example app demonstrating the capabilities of your "Naked" library, consider implementing actual button-style radio buttons.Here's a basic implementation template:
class _ButtonStyleRadioState extends State<ButtonStyleRadio> { String _selected = 'monthly'; @override Widget build(BuildContext context) { return Column( crossAxisAlignment: CrossAxisAlignment.start, children: [ const Text( 'Button-style radio options behave like radio buttons but look like buttons', style: TextStyle( fontSize: 14, color: Color(0xFF4B5563), ), ), const SizedBox(height: 16), NakedRadioGroup<String>( value: _selected, onChanged: (value) { if (value != null) { setState(() => _selected = value); } }, child: Wrap( spacing: 8, children: [ for (final option in [ {'id': 'monthly', 'label': 'Monthly'}, {'id': 'quarterly', 'label': 'Quarterly'}, {'id': 'yearly', 'label': 'Yearly'}, ]) NakedRadioButton<String>( value: option['id']!, child: Builder(builder: (context) { final isSelected = _selected == option['id']; return Container( padding: const EdgeInsets.symmetric( horizontal: 16, vertical: 8, ), decoration: BoxDecoration( borderRadius: BorderRadius.circular(4), color: isSelected ? Colors.blue : Colors.grey.shade200, ), child: Text( option['label']!, style: TextStyle( color: isSelected ? Colors.white : Colors.black, fontWeight: FontWeight.w500, ), ), ); }), ), ], ), ), ], ); } }
1131-1143: Implement the remaining placeholder classes.The
IconRadioButtonsandResponsiveRadioGridclasses also usePlaceholderwidgets. Consider implementing these examples to fully showcase the capabilities of your radio button components.Would you like me to provide implementation examples for these remaining placeholder classes to better demonstrate the component capabilities?
Also applies to: 1159-1171
218-220: Enhance the form submission feedback in FormRadioExample.The
_submitFormmethod inFormRadioExamplevalidates the form but doesn't provide user feedback about the successful submission. Consider adding a visual indicator or message to show that the form was submitted successfully.void _submitForm() { if (_formKey.currentState!.validate()) { _formKey.currentState!.save(); // Process form data + // Show a success message + ScaffoldMessenger.of(context).showSnackBar( + const SnackBar( + content: Text('Priority saved successfully'), + backgroundColor: Colors.green, + ), + ); } }
493-496: Add NakedRadioExample classes for unimplemented sections.Several sections in the
NakedRadioExamplewidget reference classes that are either placeholder implementations or entirely missing from the file. This could cause confusion for users of the example app.I can help create implementations for the missing or placeholder sections to ensure a complete demonstration of the radio button capabilities in your example app. Would you like me to generate code for any specific section?
Also applies to: 499-502, 505-508, 511-514, 517-520, 523-526
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/naked/example/flutter_01.pngis excluded by!**/*.png
📒 Files selected for processing (57)
.gitignore(1 hunks)packages/naked/.gitignore(1 hunks)packages/naked/CHANGELOG.md(1 hunks)packages/naked/LICENSE(1 hunks)packages/naked/README.md(1 hunks)packages/naked/analysis_options.yaml(1 hunks)packages/naked/example/.gitignore(1 hunks)packages/naked/example/.metadata(1 hunks)packages/naked/example/README.md(1 hunks)packages/naked/example/analysis_options.yaml(1 hunks)packages/naked/example/integration_test/app_navigation_test.dart(1 hunks)packages/naked/example/lib/error_boundary.dart(1 hunks)packages/naked/example/lib/examples/naked_accordion_example.dart(1 hunks)packages/naked/example/lib/examples/naked_avatar_example.dart(1 hunks)packages/naked/example/lib/examples/naked_button_example.dart(1 hunks)packages/naked/example/lib/examples/naked_checkbox_example.dart(1 hunks)packages/naked/example/lib/examples/naked_menu_example.dart(1 hunks)packages/naked/example/lib/examples/naked_positioning_example.dart(1 hunks)packages/naked/example/lib/examples/naked_radio_example.dart(1 hunks)packages/naked/example/lib/examples/naked_select_enhanced_example.dart(1 hunks)packages/naked/example/lib/examples/naked_select_example.dart(1 hunks)packages/naked/example/lib/examples/naked_slider_example.dart(1 hunks)packages/naked/example/lib/examples/naked_tabs_example.dart(1 hunks)packages/naked/example/lib/main.dart(1 hunks)packages/naked/example/lib/url_strategy.dart(1 hunks)packages/naked/example/lib/url_strategy_mobile.dart(1 hunks)packages/naked/example/lib/url_strategy_web.dart(1 hunks)packages/naked/example/pubspec.yaml(1 hunks)packages/naked/example/test/widget_test.dart(1 hunks)packages/naked/example/test_driver/integration_test.dart(1 hunks)packages/naked/goals.md(1 hunks)packages/naked/lib/naked.dart(1 hunks)packages/naked/lib/naked_widgets.dart(1 hunks)packages/naked/lib/src/naked_accordion.dart(1 hunks)packages/naked/lib/src/naked_avatar.dart(1 hunks)packages/naked/lib/src/naked_button.dart(1 hunks)packages/naked/lib/src/naked_checkbox.dart(1 hunks)packages/naked/lib/src/naked_menu.dart(1 hunks)packages/naked/lib/src/naked_radio_button.dart(1 hunks)packages/naked/lib/src/naked_radio_group.dart(1 hunks)packages/naked/lib/src/naked_select.dart(1 hunks)packages/naked/lib/src/naked_slider.dart(1 hunks)packages/naked/lib/src/naked_tabs.dart(1 hunks)packages/naked/lib/src/naked_widgets.dart(1 hunks)packages/naked/lib/src/utilities/naked_focus_manager.dart(1 hunks)packages/naked/lib/src/utilities/naked_portal.dart(1 hunks)packages/naked/lib/src/utilities/naked_positioning.dart(1 hunks)packages/naked/lib/src/utilities/utilities.dart(1 hunks)packages/naked/naked_component_guide.md(1 hunks)packages/naked/pubspec.yaml(1 hunks)packages/naked/research.md(1 hunks)packages/naked/test/examples/naked_avatar_example_test.dart(1 hunks)packages/naked/test/naked_accordion_focus_test.dart(1 hunks)packages/naked/test/naked_accordion_test.dart(1 hunks)packages/naked/test/naked_avatar_test.dart(1 hunks)packages/naked/test/naked_checkbox_basic_test.dart(1 hunks)packages/naked/test/naked_checkbox_test.dart(1 hunks)
🧰 Additional context used
🪛 LanguageTool
packages/naked/example/README.md
[uncategorized] ~24-~24: You might be missing the article “the” here.
Context: ...macOS, follow these steps: 1. Ensure macOS platform is supported: ```bash ...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~36-~36: The preposition “on” seems more likely in this position than the preposition “in”.
Context: ...ing - Test results will be displayed in the terminal - Any failures will be ...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_ON)
packages/naked/goals.md
[duplication] ~105-~105: Possible typo: you repeated a word.
Context: ...ations needing to deviate from Material Design** 3. Design agencies creating branded application...
(ENGLISH_WORD_REPEAT_RULE)
packages/naked/naked_component_guide.md
[duplication] ~654-~654: Possible typo: you repeated a word.
Context: ...ility**: Let consumers manage their own state 2. State Variables: Use explicit state variabl...
(ENGLISH_WORD_REPEAT_RULE)
🪛 YAMLlint (1.35.1)
packages/naked/example/pubspec.yaml
[error] 41-41: trailing spaces
(trailing-spaces)
[error] 44-44: trailing spaces
(trailing-spaces)
🔇 Additional comments (159)
.gitignore (1)
106-106: New.orbitentry added.
Adding.orbitto the ignore list is appropriate given the project's introduction of Orbit-related functionality. Ensure that this pattern does not conflict with any other tooling configurations across the repository.packages/naked/LICENSE (1)
1-22: MIT License File Added.
The license text is standard and correctly reflects the MIT License with the updated copyright year.packages/naked/example/.metadata (1)
1-31: Metadata configuration for Flutter project.
The file is well-structured with version tracking, project type, migration details, and unmanaged file directives. This setup should help the Flutter tool properly manage upgrades and migrations.packages/naked/example/.gitignore (1)
1-46: New example project.gitignoreprovided.
This file comprehensively ignores build artifacts, IDE-specific data, Flutter/Dart tool files, and other temporary files that are not needed in version control. Double-check that none of these patterns accidentally exclude files required for the example application to run correctly.packages/naked/.gitignore (1)
45-46: Orbit framework ignore rule added.
The addition of.orbit/ensures that Orbit-specific files are not tracked in version control. This is consistent with the repository’s overall strategy of ignoring generated or environment-specific directories.packages/naked/analysis_options.yaml (3)
1-2: Include Base Lint Configuration
Includingpackage:flutter_lints/flutter.yamlensures that the project inherits a robust set of Flutter-specific lint rules, promoting best practices from the start.
3-27: Comprehensive Linter Rules Setup
The linter rules cover many common pitfalls and enforce a consistent code style—including return type declarations, naming conventions, and error handling. This thorough set will help maintain code quality as the project grows.
37-43: Appropriate Analyzer Error Handling Configuration
Defining certain issues as errors (such as missing required parameters and return statements) while choosing to ignore the unnamed constructors ordering is a practical compromise that aligns with the project’s flexibility needs.packages/naked/example/test_driver/integration_test.dart (1)
1-4: Effective Integration Test Driver Setup
The file succinctly establishes an entry point for integration tests usingintegrationDriver(), aligning with Flutter’s integration testing practices.packages/naked/CHANGELOG.md (1)
1-10: Clear Initial Changelog Entry
The changelog clearly documents the initial release features, such as core functionality for the HeadlessButton component and its state management. This entry provides a solid reference point for future changes and is well structured.packages/naked/lib/src/utilities/utilities.dart (1)
1-6: Well-Documented Utility Exports
The header comments effectively explain the purpose of centralizing utility exports in this file, making it easier for developers to import necessary utilities from a single location.packages/naked/example/analysis_options.yaml (2)
1-10: Well-Documented Analyzer Configuration
The header comments provide clear context on how the analyzer works, its role in checking for errors and lints, and where to find more information. This level of documentation is very useful for developers working in the example project.
11-29: Thorough Linter Customization Instructions
The linter section contains detailed guidance on how to customize lint rules, as well as references to documentation. The commented-out examples for modifying rules serve as a useful starting point for future customization.packages/naked/lib/naked_widgets.dart (1)
1-13: Well-structured library declaration with clear documentationThe library documentation effectively explains the core philosophy of Naked widgets—providing behavior without styling. The callback-driven approach is clearly outlined with the three key principles: components provide callbacks for state changes, consumers manage their own state, and consumers control visual styling.
The code organization with exports to 'naked.dart' and 'src/naked_widgets.dart' follows good package architecture practices, separating the public API from implementation details.
packages/naked/example/test/widget_test.dart (2)
1-12: Good test setup with comprehensive commentsThe test file includes helpful comments explaining the purpose of widget testing and how to use the WidgetTester utility, which improves maintainability and serves as documentation for new contributors.
13-30: Complete smoke test implementationThe counter test follows Flutter testing best practices by:
- Building the widget tree
- Verifying initial state
- Performing user interaction
- Verifying the updated state
This provides good basic test coverage for the example app. As the Naked library grows, consider adding more comprehensive tests for each Naked component in the example app.
packages/naked/research.md (2)
1-4: Comprehensive research documentationThis introduction clearly establishes the purpose of the document as a comprehensive analysis of UI components and their various states and behaviors, which aligns well with the behavior-first approach of the Naked library.
5-68: Thorough component analysis tableThe table provides a detailed breakdown of all UI components with their interactive states, value states, and event callbacks. This research forms an excellent foundation for implementing consistent behavior patterns across the library.
The research covers both simple components (Button, Checkbox) and complex ones (Tabs, Dropdown), which aligns with the PR objectives of creating a comprehensive UI component library.
I'd recommend adding this research as part of the developer documentation for the package, as it provides valuable insights into the component architecture and behavior patterns.
packages/naked/example/pubspec.yaml (4)
1-20: Good package configurationThe package name, description, and versioning are well-defined. The decision to set
publish_to: 'none'is appropriate for an example app that shouldn't be published to pub.dev.
21-23: Appropriate SDK constraintsThe SDK constraint
'>=3.4.3 <4.0.0'follows best practices by allowing compatible minor versions while preventing potentially breaking major version upgrades.
51-65: Complete development dependenciesThe inclusion of both
flutter_testandintegration_testdemonstrates a commitment to thorough testing, which is excellent for ensuring the reliability of the example app. The use offlutter_lintsfor enforcing coding standards is also a good practice.
66-107: Well-documented Flutter configurationThe Flutter configuration section includes useful comments explaining how to add assets and custom fonts. While they're currently commented out, they provide good guidance for future additions to the example app.
packages/naked/example/README.md (2)
1-17: Well-structured introductory content with appropriate resourcesThe introduction and Getting Started sections provide a clear overview of the Headless Widgets Example App and include relevant Flutter resources for newcomers. This effectively sets the context for the example application.
39-53: Good debugging guidanceThe debugging section provides clear, actionable steps for troubleshooting integration tests. This will be helpful for developers who encounter issues while running the tests.
packages/naked/test/examples/naked_avatar_example_test.dart (2)
7-50: Comprehensive initial state testing with proper UI verificationThe first test thoroughly verifies the initial state of the NakedAvatarExample, including:
- Text elements presence
- Multiple NakedAvatar instances
- Button with correct text and icon
- Size labels
- Fallback text after network images settle
Good use of
pumpAndSettle()to allow asynchronous operations to complete before assertions, and clear finder strategies using keys, descendants, and ancestors.
52-93: Robust functionality testing with proper state change verificationThe second test effectively verifies the avatar refresh functionality:
- Captures initial image URL
- Taps the refresh button
- Verifies URL change after state update
- Checks that the URL contains expected parameters
The test uses proper widget finder techniques and correctly handles asynchronous state changes. The assertions comprehensively validate that the refresh action works as expected.
packages/naked/lib/src/naked_widgets.dart (1)
1-9: Clear exports organization following best practicesThe file efficiently exports all naked widget components, making them accessible through a single import. This follows the standard Dart practice for library organization and simplifies the API for consumers.
packages/naked/pubspec.yaml (1)
19-52: Appropriate Flutter configuration with helpful documentation referencesThe Flutter configuration section is well-structured with:
- Material design usage enabled
- Commented examples for assets and fonts
- Links to Flutter documentation for further guidance
This provides good reference for future customizations.
packages/naked/goals.md (1)
1-139: Well-structured and comprehensive vision document for Naked WidgetsThe goals document effectively communicates the purpose and vision for the Naked Widgets library. It clearly articulates the core approach of separating behavior from presentation, which aligns well with the PR objectives. The document includes detailed explanations of the callback-driven architecture, accessibility focus, and design freedom that forms the foundation of this library.
The code example on lines 74-92 effectively demonstrates the implementation approach with clear callback patterns for state management. The document also provides a thorough comparison with existing approaches and a well-defined roadmap.
🧰 Tools
🪛 LanguageTool
[duplication] ~105-~105: Possible typo: you repeated a word.
Context: ...ations needing to deviate from Material Design** 3. Design agencies creating branded application...(ENGLISH_WORD_REPEAT_RULE)
packages/naked/test/naked_checkbox_basic_test.dart (1)
1-152: Comprehensive test coverage for basic NakedCheckbox functionalityThe test suite thoroughly covers the core functionality of the NakedCheckbox component including:
- Child rendering verification
- Toggle state handling (checked/unchecked)
- Indeterminate state support
- Disabled state behavior
All test cases follow Flutter testing best practices with proper widget pumping and clear assertions. The tests demonstrate the component's callback-driven architecture as described in the PR objectives.
packages/naked/test/naked_checkbox_test.dart (4)
75-127: Good handling of skipped tests with clear explanationsThe hover state test is appropriately skipped with a detailed explanation that the issue is with the testing environment rather than the code. This demonstrates good testing practices by acknowledging tests that should exist but can't be reliably executed in the current context, while preserving the test code for future implementation.
129-176: Clear documentation for skipped pressed state testSimilar to the hover test, this pressed state test is properly skipped with clear comments explaining the environmental limitations. The preserved test code will be valuable for future implementation once the testing environment issues are resolved.
178-245: Thorough testing of disabled state behavior across multiple interaction typesThis test comprehensively verifies that disabled checkboxes don't respond to any form of interaction (tap, hover, press). The test creates proper gesture objects and verifies that state variables remain unchanged, ensuring the component behaves correctly when disabled.
247-296: Complete testing of indeterminate state transitionsThe test thoroughly verifies the checkbox's ability to handle all state transitions (null → true → false → true), ensuring the component correctly manages the tri-state behavior expected from an accessible checkbox implementation.
packages/naked/example/lib/url_strategy.dart (1)
8-9: Clever use of conditional imports for platform-specific implementationsThe conditional import pattern is an effective way to handle platform-specific code while maintaining a unified interface. The ignore comment is necessary to handle linting errors since the file paths can't both exist in a single environment.
packages/naked/test/naked_avatar_test.dart (5)
1-5: Appropriate imports setup.The imports include all necessary packages for testing the NakedAvatar component, including the Flutter test framework and Naked package.
7-25: Good basic rendering test.This test correctly verifies that the NakedAvatar component renders with its child Container as expected within the widget tree.
27-53: Well-implemented interaction test.The test properly verifies that the onPressed callback is triggered when tapping the avatar. It uses a boolean flag to track the interaction state, which is a clean approach.
55-94: Comprehensive state callback testing.This test effectively verifies both hover and pressed state callbacks by simulating the respective mouse interactions and checking that the callbacks are triggered.
96-123: Disabled state handled properly.The test correctly verifies that the onPressed callback is not triggered when the avatar is disabled, demonstrating that the disabled state works as expected.
packages/naked/test/naked_accordion_focus_test.dart (4)
7-71: Well-structured test helper function.The
buildTestApphelper creates a configurable test environment that includes a NakedAccordion within a NakedFocusManager. This approach makes the tests more maintainable by centralizing the test widget creation logic.
117-144: Item expansion toggle test is solid.This test properly verifies that pressing Enter and Space keys toggles the expansion state of the accordion items.
146-178: Focus traversal test is well implemented.The test effectively verifies that focus can move into content when expanded and then out, which is crucial for keyboard accessibility.
205-281: Disabled state test is comprehensive.This test effectively verifies that focus movement and item expansion don't work when the accordion is disabled. It checks both enter key and tapping interactions.
packages/naked/example/integration_test/app_navigation_test.dart (4)
6-22: Good integration test setup with error handling.The test correctly initializes the integration test environment and checks for exceptions during app startup, which helps identify initialization issues early.
26-45: Thorough verification of navigation elements.The test properly checks for the presence of the navigation rail, expand button, and various navigation items, ensuring the core UI is rendered correctly.
46-54: UI interaction test for navigation rail expansion.The test correctly verifies that tapping the expand button shows the expected labels, validating the expand functionality works properly.
68-100: Navigation verification with good error handling.The test properly iterates through each destination, verifies the navigation works, and checks for exceptions after each navigation, which is excellent for catching subtle integration issues.
packages/naked/lib/src/naked_radio_button.dart (6)
1-6: Appropriate imports.The imports include the necessary Flutter packages and related components from the Naked library.
7-97: Excellent documentation with comprehensive example.The class documentation provides a clear explanation of the component's purpose and a complete usage example that demonstrates all key features.
98-156: Well-designed API with clear property documentation.The component has a clean API with well-named and documented properties, making it easy for developers to understand how to use it.
158-173: Proper focus node management.The state class correctly initializes and disposes of the focus node, preventing memory leaks.
199-206: Good keyboard event handling for accessibility.The component properly handles keyboard events for Space and Enter keys, ensuring keyboard accessibility.
231-274: Good accessibility implementation with comprehensive semantics.The build method properly implements semantics for accessibility, focus management, and event handling to ensure the component is fully accessible.
packages/naked/lib/naked.dart (4)
1-14: Well-structured library documentation.The documentation clearly explains the purpose and approach of the Naked widgets library. It effectively communicates the callback-driven approach and separation of concerns between behavior and styling.
16-28: Clean component exports structure.The component exports are well-organized and clearly labeled, making it easy for consumers to understand what's available.
30-33: Good practice to document upcoming features.Documenting future components informs users about what's planned and sets expectations appropriately.
35-38: Effective comment for removed code.The comment explains why the core export was removed, which is helpful for maintainers.
packages/naked/example/lib/url_strategy_mobile.dart (2)
3-30: Well-implemented singleton pattern for UrlStrategy.The
MobileUrlStrategyclass correctly implements the singleton pattern with a private constructor and a factory method. The no-op implementations for mobile platforms are appropriate and well-documented.
32-33: Simple and effective implementation for retrieving the strategy.The
getUrlStrategy()function provides a clean interface for obtaining the platform-specific implementation.packages/naked/test/naked_accordion_test.dart (9)
6-86: Comprehensive test for basic accordion functionality.This test thoroughly validates the core functionality of the accordion component, including initial state, expansion behavior, and collapse behavior. The assertions are clear and verify important aspects of the component's behavior.
88-151: Well-structured test for multiple expansion mode.The test properly verifies the accordion's behavior in multiple expansion mode, ensuring that multiple items can be expanded simultaneously and that they can be individually collapsed.
153-188: Effective test for disabled accordion state.The test correctly verifies that a disabled accordion prevents expansion of any item.
190-241: Good test for item-level disabled state.This test properly validates that disabled items cannot be expanded while enabled items still work.
243-281: Thorough test for disabled trigger behavior.The test correctly verifies that a disabled trigger prevents expansion of its parent item.
283-325: Excellent test for custom tap override behavior.This test validates that a custom
onTaphandler correctly overrides the default toggle behavior, which is an important feature for flexibility.
327-366: Good validation of initialExpandedValues functionality.The test verifies that the accordion respects the provided initial expanded values.
368-414: Thorough test for multiple initial expanded values.This test correctly validates that multiple items can be initially expanded in multiple mode.
416-461: Important edge case test for single mode with multiple initial values.This test verifies the correct behavior when multiple initial expanded values are provided in single expansion mode.
packages/naked/example/lib/examples/naked_positioning_example.dart (1)
12-42: Well-structured state management for positioning example.The state variables and position configurations are clearly defined and organized, making the example easy to understand.
packages/naked/lib/src/naked_button.dart (8)
1-4: Great choice of imports.The imports are clean and specific, bringing in just what's needed for the naked button implementation.
5-58: Excellent documentation and example code.The documentation is thorough and clearly explains the purpose and usage of the component. The included example demonstrates all key aspects of the button's functionality, including state management for hover, press, and focus states.
59-130: Well-structured class with comprehensive properties.The class design follows best practices with clear property definitions, detailed documentation for each property, and appropriate default values. The optional parameters give developers full control over the button's behavior while maintaining sensible defaults.
131-149: Constructor implementation is clean and follows Flutter conventions.The constructor appropriately marks required parameters and provides defaults for optional ones.
151-164: Good handling of interaction state and focus.The build method correctly manages interactive state based on disabled/loading flags and properly handles the focus node creation when not provided externally.
165-172: Excellent accessibility support.The Semantics wrapper provides comprehensive accessibility support with appropriate properties for screen readers and assistive technologies.
173-196: Robust keyboard event handling.The implementation properly handles keyboard navigation and activation through space and enter keys, maintaining the pressed state visual feedback during keyboard interaction.
197-212: Complete mouse and gesture handling.The MouseRegion and GestureDetector implementation correctly manages hover and press states, with appropriate cursor changes based on the button's interactive state.
packages/naked/example/lib/examples/naked_select_enhanced_example.dart (8)
1-15: Good component organization and documentation.The example class is well-documented with a clear explanation of the enhanced features being demonstrated.
17-54: Well-structured state management.The state variables are logically organized with clear naming conventions. State variables are separated by purpose (trigger styling, option state tracking) which improves code readability.
55-92: Comprehensive user instructions and feedback.The UI provides clear instructions and explanations of the features being demonstrated, which is excellent for an example. The feedback on keyboard actions enhances the educational value of the example.
93-131: Excellent demonstration of NakedSelect capabilities.The example showcases multiple advanced features including type-ahead, focus management, and dynamic positioning. The configuration is comprehensive and demonstrates best practices for implementing complex select components.
132-147: Good feedback on selection state.The UI provides clear visual feedback when an item is selected, enhancing the user experience.
149-192: Well-implemented trigger with visual feedback.The trigger implementation demonstrates proper state handling for hover, focus, and press states with appropriate visual feedback. The container width constraint ensures consistent sizing.
194-243: Comprehensive menu implementation with scrollable container.The menu design handles constraints appropriately and implements a scrollable container to accommodate large lists. The visual design includes proper shadows and borders for a polished appearance.
245-279: Well-designed select item with clear state visualization.The select item implementation provides distinct visual feedback for selected, hovered, and focused states, enhancing usability and accessibility.
packages/naked/example/lib/url_strategy_web.dart (4)
1-8: Appropriate imports for web-specific functionality.The imports are specific to the requirements, including the JS interop for web integration and the URL strategy interface.
9-19: Good singleton implementation.The class uses the singleton pattern correctly with a private constructor and factory method for consistent instance access.
34-38: Simple and effective URL strategy initialization.The implementation correctly uses the url_strategy package to set the path URL strategy.
40-48: Clean static initialization method and factory function.The static initialize method and getUrlStrategy function provide a clean API for consumers to use the URL strategy implementation.
packages/naked/example/lib/examples/naked_avatar_example.dart (5)
1-20: Well-structured avatar example with refresh functionality.The state management is clean and the refresh mechanism works by incrementing an image index to load new avatars.
22-75: Good implementation of basic avatar with loading and error handling.The basic avatar implementation includes comprehensive loading and error handling. The refresh button provides a way to demonstrate dynamic avatar changes.
76-111: Well-implemented fallback mechanism for loading failures.The avatar fallback implementation demonstrates how to handle loading failures gracefully by displaying initials or a placeholder.
182-229: Good demonstration of different avatar sizes.The implementation effectively shows how the avatar component scales to different sizes with appropriate proportions.
230-245: Helper method improves code organization.The _buildSectionTitle helper method reduces code duplication and enhances readability.
packages/naked/lib/src/naked_checkbox.dart (1)
185-206:✅ Verification successful
Confirm tri-state toggle coverage in tests.
The toggle logic supports transitioning from indeterminate → checked → unchecked. Please verify you have test coverage for these transitions to avoid regressions.
🏁 Script executed:
#!/bin/bash # Description: Search for "NakedCheckbox" test usage handling indeterminate states rg -A 10 "NakedCheckbox" | rg -A 6 "isIndeterminate"Length of output: 8198
Test coverage for tri-state toggle transitions confirmed.
We've verified that tests in the repository (e.g., in packages/naked/test/naked_checkbox_basic_test.dart, packages/naked/test/naked_checkbox_test.dart, and in the example files) cover the indeterminate state (represented bynull) transitioning to checked, followed by toggling to unchecked. There is sufficient test coverage ensuring that the behavior of toggleValue() is protected against regressions.packages/naked/lib/src/naked_avatar.dart (1)
189-198: Good use of Semantics for accessibility.The
Semanticswidget effectively marks this component as a button with an image, improving screen reader compatibility. This approach supports best practices for accessible UI elements.packages/naked/example/lib/examples/naked_select_example.dart (2)
67-102: Impressive demonstration of the NakedSelect pattern.The usage of
NakedSelect,NakedSelectTrigger, andNakedSelectMenuclarifies how to separate the behavior from the presentation. The callbacks for open/close states, hover, and press states are logically sound. This is a strong example of behavior-first design.
425-499: Verify error states align with form validation requirements.When the select is invalid, an error is shown and a snack bar prompts the user to select a fruit. Ensure that any other potential invalid states, such as an empty item list, are gracefully handled, and consider thoroughly testing these form scenarios if not already covered.
packages/naked/lib/src/utilities/naked_focus_manager.dart (1)
92-108: Great approach for initial focus withautofocus.Using
WidgetsBinding.instance.addPostFrameCallbackto delay the focus request avoids build-phase conflicts and potential race conditions. This pattern is recommended for guaranteeing safe focus requests.packages/naked/lib/src/utilities/naked_portal.dart (2)
1-28: Thorough documentation and usage explanation.
The doc comments clearly convey the purpose and usage of theNakedPortalclass, providing helpful context on how and why it should be used.
28-46: Widget constructor and child parameter are well-defined.
The constructor contract is concise and ensures callers must provide a non-nullchild, preparing the portal for proper usage.packages/naked/example/lib/error_boundary.dart (5)
4-21: Clean and descriptive error boundary API.
TheErrorBoundaryconstructor and fields, includingcomponentName, make it straightforward to identify which component triggered the error.
23-41: Restoring the original Flutter error handler.
The code properly stores and restores the original error handler ininitStateanddispose. This is a best practice for not interfering with global error handling after this boundary is disposed.
59-95: Graceful handling of errors during build.
Catching errors in aBuilderand scheduling a post-frame callback to update the state is a solid approach to avoid “setState during build” exceptions. This helps maintain the UI consistency at runtime.
97-178: Informative error display.
The_buildErrorDisplaymethod is well structured and provides clear visual cues, stack trace information, and a reset option to recover from errors.
194-221: Simple integration ofErrorCatcher.
TheErrorCatcherstateful widget uses an internalGlobalKeyto wrap theErrorBoundary, making error reporting straightforward and centralized.packages/naked/lib/src/naked_radio_group.dart (5)
1-11: Solid foundational API.
TheNakedRadioGroupconstructor and fields provide flexible radio group configuration, includingtrapFocus,autofocus, andonEscapePressedcallbacks.
60-75: Efficient re-collection of radio values.
Using_collectRadioButtonValuesin bothdidChangeDependenciesanddidUpdateWidgetensures that radio button configurations remain consistent when dependencies or props change.
127-145: Separation of concerns.
The combination ofNakedFocusManagerandKeyboardListenerpreserves clarity by cleanly separating focus-trapping logic from key event handling.
147-164: Robust arrow key navigation logic.
Handling multiple arrow keys and preventing group navigation whenisDisabledoronChangedis null is a good pattern for accessible keyboard input.
220-252: InheritedWidget usage for radio scope.
NakedRadioGroupScopecorrectly usesupdateShouldNotifyto propagate changes, ensuring updates flow naturally to child Widgets that listen to the scope.packages/naked/naked_component_guide.md (2)
190-190: Correct usage of "Naked" name.Throughout the document, usage of "Naked" is consistent. Keep verifying that prior references to "Headless" are removed or clearly marked as deprecated to avoid confusion for new readers.
653-654: False positive detection in static analysis.The static analysis tool flagged a possible repeated word around here, but there doesn't appear to be an actual duplication. This can be safely ignored unless you identify a real repetition that's not visible in this snippet.
🧰 Tools
🪛 LanguageTool
[duplication] ~654-~654: Possible typo: you repeated a word.
Context: ...ility**: Let consumers manage their own state 2. State Variables: Use explicit state variabl...(ENGLISH_WORD_REPEAT_RULE)
packages/naked/lib/src/naked_tabs.dart (3)
198-199: Good defensive checks on tab selection.Your early return when
tabId == widget.selectedTabIdorwidget.isDisabledhelps avoid unnecessary state changes. This is a solid pattern that prevents redundant callbacks.
201-260: Robust keyboard handling.The key event logic for arrow navigation, Home/End, and ignoring events when no tabs are available is well-done. This ensures an intuitive user experience.
208-218:❓ Verification inconclusive
Consider verifying dynamic child changes.
You collect
tabIdsby visiting child elements once. In cases where children can change dynamically, the list may become outdated. Consider re-running the search after a rebuild, if dynamic tab sets are supported.
🏁 Script executed:
#!/bin/bash # Description: Search for references to dynamic content changes in widgets using NakedTab rg -A 5 'NakedTab\('Length of output: 13489
Dynamic Child Update Handling Recommendation
The current implementation in
packages/naked/lib/src/naked_tabs.dart(lines 208-218) gathers tab IDs via a one-time traversal of the widget tree. This approach may lead to outdated tab IDs when children change dynamically. Based on the tests—which mainly demonstrate static usage—it isn’t entirely clear if dynamic updates are supported. If dynamic tab sets are intended, consider triggering a new child traversal post-rebuild or integrating a listener to capture these changes.packages/naked/example/lib/examples/naked_button_example.dart (2)
226-270: Commendable reusability inBasicButtons.Your approach to storing button configurations in a list of maps and iterating over them to build UI is a concise way to reduce repetition. It showcases good maintainability if new button types are later added.
758-913: Good approach to toggling states with immediate feedback.The hot-swappable toggles for “Simple Toggle,” “Icon Toggle,” and “Theme Toggle” are well-structured. The callback usage, setState call, and user feedback are done properly.
packages/naked/README.md (3)
1-13: Well-structured introduction and feature list.The README provides a clear, concise introduction to the Naked package, effectively communicating its purpose as a behavior-first UI component library that separates state management from visual rendering. The feature list highlights key benefits without overwhelming the reader.
29-97: Excellent usage example with complete implementation.The NakedButton example is comprehensive and well-structured, demonstrating:
- State management for interaction states (hover, press, focus)
- Callback implementation pattern
- Visual rendering based on interaction states
- Animation and loading state handling
This gives developers a clear pattern to follow when implementing custom components.
146-154: Complete component list with consistent naming.The component list provides a good overview of the available widgets with consistent naming convention (all prefixed with "Naked"). The brief descriptions effectively communicate each component's purpose.
packages/naked/example/lib/main.dart (4)
25-29: Good initialization pattern for URL strategy.Initializing the URL strategy before running the app is a good practice for web applications, ensuring that the URL handling is properly set up before the app renders.
54-71: Robust URL-based routing implementation.The routing logic effectively handles both the default route and component-specific routes, extracting the component name from the URL path and finding the matching destination index. This enables deep linking to specific examples, which is great for sharing and documentation.
211-219: Good use of post-frame callbacks for state updates.Using
WidgetsBinding.instance.addPostFrameCallbackto update the state after the frame is built helps avoid potential issues with setState during layout/paint, which is a common source of Flutter errors. This is a good practice for state updates triggered by user interactions.
185-192:✅ Verification successful
Ensure updateUrl function is defined and available.
The code calls the
updateUrlfunction to update the browser URL without adding to the history stack, but there's no visible implementation of this function in the provided code. Make sure this function is defined in the importedurl_strategy.dartfile.
🏁 Script executed:
#!/bin/bash # Check if updateUrl function is defined in url_strategy.dart if grep -q "updateUrl" "packages/naked/example/lib/url_strategy.dart"; then echo "updateUrl function found in url_strategy.dart" else echo "WARNING: updateUrl function not found in url_strategy.dart" fiLength of output: 165
Verified: The
updateUrlfunction is properly defined inpackages/naked/example/lib/url_strategy.dart.The script confirmed that the
updateUrlfunction is present, so the call in_updateUrlForDestinationis valid. No further changes are necessary.packages/naked/example/lib/examples/naked_checkbox_example.dart (4)
14-17: Good responsive design approach.Using the device screen width to determine the column count is a responsive design best practice. This ensures that the UI adapts to different screen sizes appropriately.
188-201: Excellent callback implementation for state tracking.The implementation of
onStateHover,onStatePressed, andonStateFocuscallbacks is a good example of how to properly handle and visualize interaction states in Flutter. This pattern makes the state changes explicit and easy to understand.
1126-1132: Good use of fold for counting selected items.Using the
foldmethod to calculate the total selected items is a clean, functional approach to aggregating data from nested collections. This is more concise and less error-prone than imperative approaches.
1518-1623: Well-structured focus and keyboard management example.The
_buildFocusableCheckboxmethod provides a comprehensive implementation of focus management and keyboard interaction. It includes:
- State tracking for hover, focus, and press
- Callbacks for state changes
- Visual feedback for different states
- Keyboard event handling
This is an excellent example for accessibility-focused component development.
packages/naked/example/lib/examples/naked_tabs_example.dart (5)
4-76: Well-organized example structure with consistent section spacing.The main example widget provides a clear overview of all tab variants with consistent spacing and styling between sections. Using a single scrollable view with multiple examples makes the showcase easy to navigate and understand.
78-108: Clean reusable section builder method.The
_buildSectionhelper method ensures consistent styling and structure across all example sections. This is a good use of abstraction to maintain UI consistency and reduce code duplication.
561-562: Good use of vertical orientation parameter.Setting the
orientationparameter toAxis.verticalto create vertical tabs demonstrates how to use the component's flexibility. This is a good example of how the NakedTabs component can adapt to different layout requirements.
868-901: Good border radius handling for button tabs.Calculating the appropriate border radius based on element position (first, last) ensures a coherent visual appearance. This is a good attention to detail for UI implementation.
1194-1271: Well-implemented responsive tab list with wrapping.Using
Wrapfor the tab list ensures it works well on smaller screens by wrapping to new lines when needed. The pill-style tabs with clear active states provide good visual feedback.packages/naked/lib/src/naked_slider.dart (1)
1-254: Handle edge case whenmin == max
If the user setsminandmaxto the same value, the slider range is effectively zero and might cause unexpected behavior, such as division by zero or a stuck thumb.Please verify whether the current clamp logic gracefully handles this scenario. You can run a quick sanity check by setting
minandmaxto the same value and reviewing behavior in the example or tests.packages/naked/example/lib/examples/naked_accordion_example.dart (1)
601-1193: Accordions appear logically constructed
Your nested and card-style accordions provide substantial flexibility. The straightforward usage ofNakedAccordionwith multiple triggers and items is clean and well-structured.packages/naked/lib/src/utilities/naked_positioning.dart (5)
5-69: Great use of a dedicated class for alignment definitions.This
AnchorPositionclass is clearly expressed and covers a wide range of positioning anchors. The self-documenting constants (e.g.,topCenter,bottomCenter) improve code readability and maintainability.
71-103: Comprehensive documentation of theNakedPositioningwidget.The detailed explanation of usage, algorithmic steps, and example snippet help clarify the widget's behavior. This fosters easy adoption by developers.
224-274: Clear position calculation logic.Your
_calculatePositionmethod effectively enumerates all preferred anchor points, scores them for visibility, and applies constraints. The step-by-step approach is logical and well-organized.
290-311: Robust viewport scoring mechanism.By calculating the proportion of the widget that remains within the screen bounds, you ensure optimal placement. This partial-fit approach is nicely suited for adaptive UIs.
341-363: Well-handled staged rendering approach.Using
_AdaptiveSizeMeasurebeforePositionedensures correct measurements for dynamic content. This pattern effectively avoids layout jank, especially when widget sizes may change.packages/naked/lib/src/naked_accordion.dart (5)
3-10: Enum usage is straightforward.Defining
NakedAccordionTypewithsingleandmultipleclarifies the core expansion modes. The code is succinct and self-explanatory.
93-140: Clear and flexible configuration.The
NakedAccordionwidget provides well-defined parameters (liketype,initialExpandedValues, etc.) and thoroughly documents them. This fosters easy integration into diverse UI scenarios.
142-216: Good management of expanded items.Your state logic for
_expandedValuesis concise. It's also elegant that you reset expansions if the accordion type changes from multiple to single, preserving just one expanded item.
257-421: Neat separation of responsibilities inNakedAccordionTrigger.The component focuses on user interaction (hover, pressed, focus) and defers the actual expansion logic to the ancestor scope. This design improves testability and reusability.
528-619: Straightforward item encapsulation.The
NakedAccordionItemaggregates trigger and content neatly, with an_NakedAccordionItemScopefor internal state. This consistent pattern helps ensure each piece is easily maintainable.packages/naked/example/lib/examples/naked_slider_example.dart (8)
1-34: Clear top-level organization and introduction of slider examples.Using
_buildSectionto group each slider scenario is a clean approach that keeps the UI well-structured.
116-215: Good demonstration of a simplistic “basic slider.”The custom track rendering, thumb styling, and use of
NakedSliderStatehighlight the library’s unopinionated nature while providing a functional example.
218-351: Nice real-time visual feedback inSliderWithValue.Displaying the current percentage fosters an intuitive user experience. Reusing the same approach (a
Builder+ local style logic) is consistent and easy to follow.
353-549: Range slider logic is well-executed.Splitting the min and max thumbs into separate
NakedSliderwidgets is clever, and the conditionif (newValue < _maxValue)/if (newValue > _minValue)effectively enforces consistent range constraints.
551-709: Excellent showcase of color variants.Using a simple map for variant data is a practical way to loop over multiple slider styles without code duplication. This organizes them neatly in one widget.
711-968: Informative icon-based sliders for volume and brightness.The minimal icon usage with color-coded tracks helps illustrate how other UI elements can be combined with unstyled slider logic. The code is straightforward and accessible.
970-1149: Stepped slider design is a great demonstration.The discrete steps, labeled markers, and corresponding text reinforce the adaptability of
NakedSliderfor more advanced requirements like skill levels. This portion is well-documented.
1150-1312: Clever approach to a vertical slider.By flipping alignment to the bottom, you deliver a clear illustration of a non-horizontal orientation. The helper labels and a separate value display at the end enhance usability.
packages/naked/example/lib/examples/naked_radio_example.dart (4)
619-648: Verify the hover state access through NakedRadioGroupScope.The code is accessing
radioGroup?.value == option['id']to determine if the option is hovered, but this is comparing the selected value, not hover state. This approach might not correctly capture hover state.Consider refactoring to properly track the hover state of each radio button:
- final isHovered = radioGroup?.value == option['id']; + // Typically hover state should be managed by the individual radio button + bool isHovered = false; + return StatefulBuilder( + builder: (context, setHoverState) { + return NakedRadioButton<String>( + value: option['id']!, + onStateHover: (hovering) => + setHoverState(() => isHovered = hovering), + child: Row(Please check if
NakedRadioGroupScopeactually provides hover state information or if this is intended to be handled differently in your implementation.
1-4: The example successfully demonstrates the behavior-first approach of the Naked library.This example file nicely showcases how the Naked library separates behavior from presentation by providing unstyled components with rich interaction states. The examples demonstrate various ways to customize the appearance while leveraging the built-in accessibility and state management features.
1262-1275: Excellent keyboard navigation implementation in the FocusRadioExample.The focus example includes proper keyboard navigation including tab for focus, arrow keys for navigation, space for selection, and escape to clear selection. This demonstrates good accessibility practices and aligns well with the PR objective of creating behavior-first UI components with built-in accessibility features.
1201-1238: Well-designed usage instructions in the keyboard navigation example.The visual card explaining keyboard navigation provides clear instructions for users, enhancing the usability of the component. This is a good practice that helps communicate the component's capabilities to developers who will use the library.
| void _findRadioButtonValues(BuildContext context) { | ||
| // Reset the list | ||
| _buttonValues.clear(); | ||
|
|
||
| // Find all radio button values in a simple way | ||
| void visitor(Element element) { | ||
| // Skip nested radio groups | ||
| if (element.widget is NakedRadioGroupScope<T>) { | ||
| return; | ||
| } | ||
|
|
||
| // Check for a radio button by custom matching | ||
| if (element.widget.runtimeType | ||
| .toString() | ||
| .contains('NakedRadioButton<$T>')) { | ||
| try { | ||
| // We need to use reflection to get the value field | ||
| final dynamic radioButton = element.widget; | ||
| // Only proceed if this widget has a 'value' getter that returns T | ||
| if (radioButton != null) { | ||
| final value = radioButton.value; | ||
| if (value is T && !_buttonValues.contains(value)) { | ||
| _buttonValues.add(value); | ||
| } | ||
| } | ||
| } catch (e) { | ||
| // Ignore errors during reflection | ||
| } | ||
| } | ||
|
|
||
| // Continue searching | ||
| element.visitChildren(visitor); | ||
| } | ||
|
|
||
| // Start the search from this widget's element | ||
| context.visitChildElements(visitor); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reliance on runtime type check for locating radio buttons.
Currently, _findRadioButtonValues depends on runtimeType.toString().contains('NakedRadioButton<$T>'), which can break if the class name or generic signature changes, or if code is minified. Consider using a more robust mechanism (e.g., passing a specialized widget key or referencing an interface) to gather radio buttons.
- if (element.widget.runtimeType.toString().contains('NakedRadioButton<$T>')) {
+ // Use a more secure, explicit identification approach, e.g.:
+ if (element.widget is NakedRadioButton<T>) {📝 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.
| void _findRadioButtonValues(BuildContext context) { | |
| // Reset the list | |
| _buttonValues.clear(); | |
| // Find all radio button values in a simple way | |
| void visitor(Element element) { | |
| // Skip nested radio groups | |
| if (element.widget is NakedRadioGroupScope<T>) { | |
| return; | |
| } | |
| // Check for a radio button by custom matching | |
| if (element.widget.runtimeType | |
| .toString() | |
| .contains('NakedRadioButton<$T>')) { | |
| try { | |
| // We need to use reflection to get the value field | |
| final dynamic radioButton = element.widget; | |
| // Only proceed if this widget has a 'value' getter that returns T | |
| if (radioButton != null) { | |
| final value = radioButton.value; | |
| if (value is T && !_buttonValues.contains(value)) { | |
| _buttonValues.add(value); | |
| } | |
| } | |
| } catch (e) { | |
| // Ignore errors during reflection | |
| } | |
| } | |
| // Continue searching | |
| element.visitChildren(visitor); | |
| } | |
| // Start the search from this widget's element | |
| context.visitChildElements(visitor); | |
| } | |
| void _findRadioButtonValues(BuildContext context) { | |
| // Reset the list | |
| _buttonValues.clear(); | |
| // Find all radio button values in a simple way | |
| void visitor(Element element) { | |
| // Skip nested radio groups | |
| if (element.widget is NakedRadioGroupScope<T>) { | |
| return; | |
| } | |
| // Check for a radio button by custom matching | |
| // Use a more secure, explicit identification approach, e.g.: | |
| if (element.widget is NakedRadioButton<T>) { | |
| try { | |
| // We need to use reflection to get the value field | |
| final dynamic radioButton = element.widget; | |
| // Only proceed if this widget has a 'value' getter that returns T | |
| if (radioButton != null) { | |
| final value = radioButton.value; | |
| if (value is T && !_buttonValues.contains(value)) { | |
| _buttonValues.add(value); | |
| } | |
| } | |
| } catch (e) { | |
| // Ignore errors during reflection | |
| } | |
| } | |
| // Continue searching | |
| element.visitChildren(visitor); | |
| } | |
| // Start the search from this widget's element | |
| context.visitChildElements(visitor); | |
| } |
| Widget _buildCardContainer({required String title, required Widget child}) { | ||
| return Container( | ||
| width: double.infinity, | ||
| padding: const EdgeInsets.all(16), | ||
| decoration: BoxDecoration( | ||
| color: Colors.white, | ||
| borderRadius: BorderRadius.circular(8), | ||
| boxShadow: [ | ||
| BoxShadow( | ||
| color: Colors.black.withOpacity(0.05), | ||
| blurRadius: 2, | ||
| offset: const Offset(0, 1), | ||
| ), | ||
| ], | ||
| ), | ||
| child: Column( | ||
| crossAxisAlignment: CrossAxisAlignment.start, | ||
| children: [ | ||
| Text( | ||
| title, | ||
| style: TextStyle( | ||
| fontSize: 16, | ||
| fontWeight: FontWeight.w500, | ||
| color: Colors.grey[700], | ||
| ), | ||
| ), | ||
| const SizedBox(height: 12), | ||
| child, | ||
| ], | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid duplicating lofty card container logic across multiple components.
Both _buildCardContainer and _buildSectionTitle appear in multiple classes. Extracting these into a shared widget or utility function can uphold the DRY principle and simplify maintenance.
- Widget _buildCardContainer({required String title, required Widget child}) {
+ // Consider factoring this into a shared widget or extension in a separate file,
+ // so multiple classes can reuse the same style or override as needed.Committable suggestion skipped: line range outside the PR's diff.
| // Determine colors based on state - mimicking Tailwind focus:ring, text-blue-500 etc. | ||
| Color effectiveBorderColor = isDisabled ? Colors.grey.shade300 : color; | ||
| Color effectiveBgColor = isDisabled | ||
| ? Colors.grey.shade200 | ||
| : (isChecked || isIndeterminate) | ||
| ? color | ||
| : Colors.white; | ||
| Color checkIconColor = isDisabled ? Colors.grey.shade500 : Colors.white; | ||
| Color? focusRingColor; | ||
|
|
||
| // Adjust colors for enabled states using alphaBlend for darkening/lightening | ||
| if (!isDisabled) { | ||
| // --- Focus State --- (Mimics focus ring) | ||
| if (isFocused) { | ||
| // Darken border slightly for focus | ||
| effectiveBorderColor = | ||
| Color.alphaBlend(Colors.black.withOpacity(0.3), color); | ||
| // Create a semi-transparent outer ring color | ||
| focusRingColor = color.withOpacity(0.3); | ||
| } | ||
|
|
||
| // --- Hover State --- (Subtle background/border change) | ||
| if (isHovered) { | ||
| effectiveBorderColor = | ||
| Color.alphaBlend(Colors.black.withOpacity(0.2), color); | ||
| if (!isChecked && !isIndeterminate) { | ||
| // Light background tint on hover when unchecked | ||
| effectiveBgColor = | ||
| Color.alphaBlend(color.withOpacity(0.05), Colors.white); | ||
| } | ||
| } | ||
|
|
||
| // --- Pressed State --- (More noticeable background/border change) | ||
| if (isPressed) { | ||
| effectiveBorderColor = | ||
| Color.alphaBlend(Colors.black.withOpacity(0.4), color); | ||
| if (!isChecked && !isIndeterminate) { | ||
| // Darker background tint on press when unchecked | ||
| effectiveBgColor = | ||
| Color.alphaBlend(color.withOpacity(0.1), Colors.white); | ||
| } else { | ||
| // Darken the main color slightly when pressed & checked/indeterminate | ||
| effectiveBgColor = | ||
| Color.alphaBlend(Colors.black.withOpacity(0.2), color); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Icon for checkmark or indeterminate state (Using Material Icons) | ||
| Widget? checkMark; | ||
| if (isIndeterminate) { | ||
| // Web Reference: <Minus className="w-4 h-4 text-white" /> | ||
| // Use Material Icons.remove for the minus symbol | ||
| checkMark = Icon(Icons.remove, size: 16, color: checkIconColor); | ||
| } else if (isChecked) { | ||
| // Web Reference: <Check className="w-4 h-4 text-white" /> | ||
| // Use Material Icons.check for the check symbol | ||
| checkMark = Icon(Icons.check, size: 16, color: checkIconColor); | ||
| } | ||
|
|
||
| // Base container with border and background | ||
| Widget checkboxVisual = Container( | ||
| width: 20, // w-5 | ||
| height: 20, // h-5 | ||
| decoration: BoxDecoration( | ||
| color: effectiveBgColor, | ||
| // Web Reference: rounded | ||
| borderRadius: BorderRadius.circular(4), | ||
| border: Border.all( | ||
| color: effectiveBorderColor, | ||
| width: 1.5, // Slightly thicker border | ||
| ), | ||
| ), | ||
| child: checkMark, // Place the icon inside | ||
| ); | ||
|
|
||
| // Add focus ring effect using an outer container if focused | ||
| if (focusRingColor != null) { | ||
| checkboxVisual = Container( | ||
| // Apply padding to create space for the ring | ||
| padding: const EdgeInsets.all(2.0), | ||
| decoration: BoxDecoration( | ||
| // Match the checkbox shape or make slightly larger if needed | ||
| borderRadius: BorderRadius.circular(6), | ||
| border: | ||
| Border.all(color: focusRingColor, width: 1.5), // The focus ring | ||
| ), | ||
| // The actual checkbox visual goes inside the ring container | ||
| child: checkboxVisual, | ||
| ); | ||
| } | ||
|
|
||
| // Use AnimatedContainer for smooth transitions between states | ||
| return AnimatedContainer( | ||
| duration: const Duration(milliseconds: 100), // Quick transition | ||
| // The decoration is now applied to the inner container(s) | ||
| // so the AnimatedContainer just handles the smooth transition. | ||
| child: checkboxVisual, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Code duplication in checkbox visual builders.
The _buildDefaultCheckboxVisual method is duplicated across different widget classes. Consider extracting this into a shared utility class or widget to maintain DRY (Don't Repeat Yourself) principles.
Create a utility class or file to share this common functionality:
// In a file like checkbox_visual_utilities.dart
class CheckboxVisualBuilder {
static Widget buildDefaultCheckboxVisual({
required bool isChecked,
bool isIndeterminate = false,
bool isHovered = false,
bool isPressed = false,
bool isFocused = false,
bool isDisabled = false,
required Color color,
}) {
// Implementation moved from _buildDefaultCheckboxVisual
}
}
// Then in your widget files:
import 'checkbox_visual_utilities.dart';
// Replace the duplicated methods with:
CheckboxVisualBuilder.buildDefaultCheckboxVisual(
isChecked: _isChecked,
isIndeterminate: _isIndeterminate,
isHovered: _isHovered,
isPressed: _isPressed,
isFocused: _isFocused,
color: Colors.blue,
)| Widget _buildPriorityOption( | ||
| FormFieldState<String> state, String value, String label, Color color) { | ||
| bool isHovered = false; | ||
| bool isPressed = false; | ||
|
|
||
| return StatefulBuilder( | ||
| builder: (context, setInnerState) { | ||
| final isSelected = state.value == value; | ||
|
|
||
| return Row( | ||
| children: [ | ||
| NakedRadioButton<String>( | ||
| value: value, | ||
| semanticLabel: label, | ||
| onStateHover: (isHovered) => | ||
| setInnerState(() => isHovered = isHovered), | ||
| onStatePressed: (isPressed) => | ||
| setInnerState(() => isPressed = isPressed), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix state management in FormRadioExample's _buildPriorityOption method.
Same issue with variable scoping and state management.
Widget _buildPriorityOption(
FormFieldState<String> state, String value, String label, Color color) {
- bool isHovered = false;
- bool isPressed = false;
return StatefulBuilder(
builder: (context, setInnerState) {
+ bool isHovered = false;
+ bool isPressed = false;
final isSelected = state.value == value;
return Row(
children: [
NakedRadioButton<String>(
value: value,
semanticLabel: label,
- onStateHover: (isHovered) =>
- setInnerState(() => isHovered = isHovered),
- onStatePressed: (isPressed) =>
- setInnerState(() => isPressed = isPressed),
+ onStateHover: (hovering) =>
+ setInnerState(() => isHovered = hovering),
+ onStatePressed: (pressing) =>
+ setInnerState(() => isPressed = pressing),📝 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.
| Widget _buildPriorityOption( | |
| FormFieldState<String> state, String value, String label, Color color) { | |
| bool isHovered = false; | |
| bool isPressed = false; | |
| return StatefulBuilder( | |
| builder: (context, setInnerState) { | |
| final isSelected = state.value == value; | |
| return Row( | |
| children: [ | |
| NakedRadioButton<String>( | |
| value: value, | |
| semanticLabel: label, | |
| onStateHover: (isHovered) => | |
| setInnerState(() => isHovered = isHovered), | |
| onStatePressed: (isPressed) => | |
| setInnerState(() => isPressed = isPressed), | |
| Widget _buildPriorityOption( | |
| FormFieldState<String> state, String value, String label, Color color) { | |
| return StatefulBuilder( | |
| builder: (context, setInnerState) { | |
| bool isHovered = false; | |
| bool isPressed = false; | |
| final isSelected = state.value == value; | |
| return Row( | |
| children: [ | |
| NakedRadioButton<String>( | |
| value: value, | |
| semanticLabel: label, | |
| onStateHover: (hovering) => | |
| setInnerState(() => isHovered = hovering), | |
| onStatePressed: (pressing) => | |
| setInnerState(() => isPressed = pressing), |
| Widget _buildStyleOption(String value, Color color, String label) { | ||
| bool isHovered = false; | ||
| bool isPressed = false; | ||
| bool isFocused = false; | ||
|
|
||
| return StatefulBuilder( | ||
| builder: (context, setInnerState) { | ||
| final isSelected = _selectedOption == value; | ||
|
|
||
| return Column( | ||
| children: [ | ||
| NakedRadioButton<String>( | ||
| value: value, | ||
| semanticLabel: label, | ||
| onStateHover: (isHovered) => | ||
| setInnerState(() => isHovered = isHovered), | ||
| onStatePressed: (isPressed) => | ||
| setInnerState(() => isPressed = isPressed), | ||
| onStateFocus: (isFocused) => | ||
| setInnerState(() => isFocused = isFocused), | ||
| child: AnimatedContainer( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix the same state management issue in the StyledRadioExample.
Similar to the previous issue, the variables are initialized outside the StatefulBuilder's builder but then attempts to be modified within the callbacks.
Widget _buildStyleOption(String value, Color color, String label) {
- bool isHovered = false;
- bool isPressed = false;
- bool isFocused = false;
return StatefulBuilder(
builder: (context, setInnerState) {
+ bool isHovered = false;
+ bool isPressed = false;
+ bool isFocused = false;
final isSelected = _selectedOption == value;
return Column(
children: [
NakedRadioButton<String>(
value: value,
semanticLabel: label,
- onStateHover: (isHovered) =>
- setInnerState(() => isHovered = isHovered),
- onStatePressed: (isPressed) =>
- setInnerState(() => isPressed = isPressed),
- onStateFocus: (isFocused) =>
- setInnerState(() => isFocused = isFocused),
+ onStateHover: (hovering) =>
+ setInnerState(() => isHovered = hovering),
+ onStatePressed: (pressing) =>
+ setInnerState(() => isPressed = pressing),
+ onStateFocus: (focusing) =>
+ setInnerState(() => isFocused = focusing),📝 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.
| Widget _buildStyleOption(String value, Color color, String label) { | |
| bool isHovered = false; | |
| bool isPressed = false; | |
| bool isFocused = false; | |
| return StatefulBuilder( | |
| builder: (context, setInnerState) { | |
| final isSelected = _selectedOption == value; | |
| return Column( | |
| children: [ | |
| NakedRadioButton<String>( | |
| value: value, | |
| semanticLabel: label, | |
| onStateHover: (isHovered) => | |
| setInnerState(() => isHovered = isHovered), | |
| onStatePressed: (isPressed) => | |
| setInnerState(() => isPressed = isPressed), | |
| onStateFocus: (isFocused) => | |
| setInnerState(() => isFocused = isFocused), | |
| child: AnimatedContainer( | |
| Widget _buildStyleOption(String value, Color color, String label) { | |
| return StatefulBuilder( | |
| builder: (context, setInnerState) { | |
| bool isHovered = false; | |
| bool isPressed = false; | |
| bool isFocused = false; | |
| final isSelected = _selectedOption == value; | |
| return Column( | |
| children: [ | |
| NakedRadioButton<String>( | |
| value: value, | |
| semanticLabel: label, | |
| onStateHover: (hovering) => | |
| setInnerState(() => isHovered = hovering), | |
| onStatePressed: (pressing) => | |
| setInnerState(() => isPressed = pressing), | |
| onStateFocus: (focusing) => | |
| setInnerState(() => isFocused = focusing), | |
| child: AnimatedContainer( | |
| // ... rest of the widget code | |
| ), | |
| ), | |
| ], | |
| ); | |
| }, | |
| ); | |
| } |
| bool isHovered = false; | ||
| bool isPressed = false; | ||
|
|
||
| return StatefulBuilder( | ||
| builder: (context, setInnerState) { | ||
| final isSelected = _selectedOption == value; | ||
|
|
||
| return Row( | ||
| children: [ | ||
| NakedRadioButton<String>( | ||
| value: value, | ||
| onStateHover: (isHovered) => | ||
| setInnerState(() => isHovered = isHovered), | ||
| onStatePressed: (isPressed) => | ||
| setInnerState(() => isPressed = isPressed), | ||
| onStateFocus: (isFocused) => {}, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider updating state management approach in the _buildRadioOption method.
The local variables isHovered and isPressed are initialized at lines 37-38 but the way they're being updated in the callbacks doesn't actually modify these variables. The setInnerState closure is capturing separate instances of these variables.
- Widget _buildRadioOption(String value, String label) {
- bool isHovered = false;
- bool isPressed = false;
+ Widget _buildRadioOption(String value, String label) {
return StatefulBuilder(
builder: (context, setInnerState) {
+ bool isHovered = false;
+ bool isPressed = false;
final isSelected = _selectedOption == value;
return Row(
children: [
NakedRadioButton<String>(
value: value,
onStateHover: (isHovered) =>
setInnerState(() => isHovered = isHovered),
onStatePressed: (isPressed) =>
setInnerState(() => isPressed = isPressed),Or alternatively, use different variable names in the callbacks:
Widget _buildRadioOption(String value, String label) {
bool isHovered = false;
bool isPressed = false;
return StatefulBuilder(
builder: (context, setInnerState) {
final isSelected = _selectedOption == value;
return Row(
children: [
NakedRadioButton<String>(
value: value,
- onStateHover: (isHovered) =>
- setInnerState(() => isHovered = isHovered),
- onStatePressed: (isPressed) =>
- setInnerState(() => isPressed = isPressed),
+ onStateHover: (hovering) =>
+ setInnerState(() => isHovered = hovering),
+ onStatePressed: (pressing) =>
+ setInnerState(() => isPressed = pressing),📝 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.
| bool isHovered = false; | |
| bool isPressed = false; | |
| return StatefulBuilder( | |
| builder: (context, setInnerState) { | |
| final isSelected = _selectedOption == value; | |
| return Row( | |
| children: [ | |
| NakedRadioButton<String>( | |
| value: value, | |
| onStateHover: (isHovered) => | |
| setInnerState(() => isHovered = isHovered), | |
| onStatePressed: (isPressed) => | |
| setInnerState(() => isPressed = isPressed), | |
| onStateFocus: (isFocused) => {}, | |
| bool isHovered = false; | |
| bool isPressed = false; | |
| return StatefulBuilder( | |
| builder: (context, setInnerState) { | |
| final isSelected = _selectedOption == value; | |
| return Row( | |
| children: [ | |
| NakedRadioButton<String>( | |
| value: value, | |
| onStateHover: (hovering) => | |
| setInnerState(() => isHovered = hovering), | |
| onStatePressed: (pressing) => | |
| setInnerState(() => isPressed = pressing), | |
| onStateFocus: (isFocused) => {}, | |
| ), | |
| // ... other widget code | |
| ], | |
| ); | |
| }, | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
packages/naked/lib/src/naked_slider.dart (1)
1-504: 💡 Verification agent🧩 Analysis chain
Verify RTL behavior for drag operations
While keyboard navigation correctly handles RTL text direction, ensure that drag operations also respect RTL layout.
The keyboard navigation properly inverts left/right arrows in RTL mode, but it's unclear if the drag calculation does the same.
Test the component with RTL text direction to ensure dragging works intuitively (drag right should decrease value in RTL).
🏁 Script executed:
#!/bin/bash # Check if the code adjusts drag calculations for RTL layout direction rg -A 5 "textDirection|Directionality" "packages/naked/lib/src/naked_slider.dart"Length of output: 416
RTL Drag Operation Handling Needs Fix
After verifying the code, the RTL inversion is only applied during keyboard navigation. In the drag update handler (_handleDragUpdate), the drag delta is calculated directly (using
dragDelta.dx) without considering the RTL layout. This means that while keyboard inputs are correctly inverted in RTL mode, drag operations remain unaffected.Action Items:
- Review and update
_handleDragUpdateinpackages/naked/lib/src/naked_slider.dartto incorporate RTL behavior (e.g., invertdragDelta.dxfor RTL horizontal sliders).- Manually test the slider in an RTL context to ensure that dragging right decreases the value as expected.
♻️ Duplicate comments (9)
packages/naked/lib/src/naked_menu.dart (1)
508-511: ** Implement outside-tap handling to respectcloseOnClickOutside**
This code currently prevents tap events from propagating but does not close the menu, even ifcloseOnClickOutsideis true. The same concern was raised in a past review comment. Please integrate a condition to close the menu on outside taps if the parameter is enabled:- onTap: () {}, // Prevent taps from reaching background + onTap: () { + if (menuWidget.closeOnClickOutside) { + menuScope.closeMenu(); + } + },Also applies to: 523-524
packages/naked/lib/src/naked_avatar.dart (1)
185-185: 🛠️ Refactor suggestionRefactor to manage the
FocusNodelifecycle explicitly
Currently, a newFocusNodeis created duringbuild. Storing and disposing of it in_NakedAvatarState(if none is provided) will prevent resource leaks and preserve focus across builds.class _NakedAvatarState extends State<NakedAvatar> { + FocusNode? _internalFocusNode; @override void initState() { super.initState(); + if (widget.focusNode == null) { + _internalFocusNode = FocusNode(); + } } @override void dispose() { + _internalFocusNode?.dispose(); super.dispose(); } @override Widget build(BuildContext context) { - final effectiveFocusNode = widget.focusNode ?? FocusNode(); + final effectiveFocusNode = widget.focusNode ?? _internalFocusNode!; ... } }packages/naked/example/lib/examples/naked_button_example.dart (1)
915-946: Refactor repeated_buildCardContainermethods.
This pattern appears in several components (e.g.,ToggleButtons,LoadingButtons,StyledButtons). Extracting it as a shared widget or utility function helps uphold the DRY principle, avoiding the same repeated container logic.Also applies to: 1192-1223, 1450-1481
packages/naked/README.md (1)
107-108: Documentation link possibly invalidIt appears that the documentation link at
.context/plan/naked_component_development_guide.mdmay not be accessible in the published package, mirroring a previous concern.packages/naked/example/lib/examples/naked_checkbox_example.dart (1)
315-316: Repeated_buildDefaultCheckboxVisualimplementationsThe
_buildDefaultCheckboxVisualmethod (and slight variations of it) is duplicated multiple times across components (e.g., BasicCheckboxStates, ParentChildCheckboxes, StyledCheckboxVariant, and SelectionExample). Consider extracting this into a shared utility or widget to reduce duplication.packages/naked/lib/src/naked_select.dart (1)
285-288: Dispose focus nodes in_clearRegisteredValuesThe
_itemFocusNodeslist is cleared without disposing eachFocusNode, potentially causing memory leaks. Disposing them in_clearRegisteredValues()(or when no longer needed) has been previously suggested.packages/naked/example/lib/examples/naked_radio_example.dart (3)
37-51: Same local state scoping issue as previously flagged.
Duplicate of an earlier review's feedback: the variablesisHoveredandisPresseddeclared outside theStatefulBuildercan be overshadowed in the closure’s scope. Consider placing them fully inside thebuilderso they reflect the correct state values.
127-144: Same local state scoping issue as previously flagged.
Similar to the previous comment, the variables declared outside theStatefulBuilderin_buildStyleOptionmight lead to confusion about which scope is updated. Aligning names or moving them into the closure helps avoid overshadowing.
291-307: Same local state scoping issue as previously flagged.
Again, you’re initializingbool isHoveredandbool isPressedoutside thebuilder, then updating them inside. Consider fully scoping these variables inside thebuildercallbacks to ensure proper state handling.
🧹 Nitpick comments (21)
packages/naked/example/lib/examples/naked_accordion_example.dart (3)
97-224: Potential duplication of single-mode behavior.Within
_BasicAccordion, you are manually collapsing other items (lines 153–160) even though the accordion is already configured withNakedAccordionType.single(line 146). The built-in single-mode logic inNakedAccordionwill handle closing other items by default.-// Lines 153-160 manually set other items to false +// Consider removing lines 153-160 to rely on single-mode logic alone
578-829: Effective demonstration of nested expansions.The
_NestedAccordiondemonstrates a nested structure well. For highly nested content, watch for performance overhead, but for an example, this is fine.
975-1193: Flexible card-based offerings.The
_CardAccordionelegantly adapts to different screen widths via row or column layout. Consider using a GridView or ListView for dynamic, large-scale usage, but for a small set of card items, this is suitable.packages/naked/lib/src/naked_accordion.dart (1)
528-620: Good item structure but consider clarifying default vs. local expansions.
NakedAccordionItemtracks an optional localisExpandedvs. the parent accordion’s state. This is flexible but can lead to confusion. A short note in the docs might help clarify usage.packages/naked/lib/src/naked_slider.dart (4)
115-212: Consider additional parameter validationWhile you correctly validate that
min < maxwith an assertion, consider adding validation forkeyboardStepandlargeKeyboardStepto ensure they're positive values.}) : assert(min < max, 'min must be less than max'); + assert(keyboardStep > 0, 'keyboardStep must be positive'); + assert(largeKeyboardStep > 0, 'largeKeyboardStep must be positive');
303-329: Handle potential performance bottleneck in drag updatesThe
_handleDragUpdatemethod is called frequently during dragging and recomputes the RenderBox on each update.Consider caching the RenderBox size during
_handleDragStartto avoid recalculating it on every update:void _handleDragStart(DragStartDetails details) { if (widget.isDisabled || widget.onChanged == null) return; + final RenderBox? box = context.findRenderObject() as RenderBox?; + _dragExtent = box != null ? + (widget.direction == SliderDirection.horizontal ? box.size.width : box.size.height) : null; setState(() { _isDragging = true; _dragStartPosition = details.globalPosition; _dragStartValue = widget.value; }); widget.onDraggingState?.call(true); widget.onDragStart?.call(); } void _handleDragUpdate(DragUpdateDetails details) { if (!_isDragging || widget.isDisabled || widget.onChanged == null) return; // Calculate the drag delta in the proper direction final Offset currentPosition = details.globalPosition; final Offset dragDelta = currentPosition - _dragStartPosition!; - // Get the RenderBox of the slider - final RenderBox? box = context.findRenderObject() as RenderBox?; - if (box == null) return; + if (_dragExtent == null) return; // Convert the drag delta to a value change - double dragExtent = widget.direction == SliderDirection.horizontal - ? box.size.width - : box.size.height; + double dragExtent = _dragExtent!;Don't forget to add the
_dragExtentfield to the class:double? _dragExtent;
410-426: Enhance accessibility semantics with more contextThe semantics implementation provides basic percentage information but could be enhanced with more context.
Consider adding an optional semantic format string to provide more meaningful context:
+ /// Optional format string for accessibility semantics. + /// + /// Use {value} and {percent} placeholders, e.g. "{value}°C" or "{percent}% complete" + /// If not provided, defaults to percentage value. + final String? semanticFormat; // In constructor: + this.semanticFormat, // In build method: + String getSemanticValue() { + final double percentage = widget.max > widget.min + ? ((widget.value - widget.min) / (widget.max - widget.min)) * 100 + : 0.0; + + if (widget.semanticFormat != null) { + return widget.semanticFormat! + .replaceAll('{value}', widget.value.toStringAsFixed(1)) + .replaceAll('{percent}', percentage.round().toString()); + } + + return widget.semanticLabel ?? '${percentage.round()}%'; + } // Then use getSemanticValue() instead of the direct calculation
476-479: Consider debouncing callback notificationsFor high-frequency events like dragging, consider debouncing the
onChangedcallback notifications to prevent excessive rebuilds.You could add a throttle mechanism for drag updates to limit the frequency of notifications:
import 'dart:async'; // Add to class fields: Timer? _throttleTimer; double? _lastReportedValue; // In _handleDragUpdate: void _handleDragUpdate(DragUpdateDetails details) { // Existing code... double newValue = _normalizeValue(_dragStartValue! + valueDelta); if (newValue != widget.value) { // Throttle notifications if (_throttleTimer == null) { widget.onChanged?.call(newValue); _lastReportedValue = newValue; _throttleTimer = Timer(const Duration(milliseconds: 16), () { _throttleTimer = null; // If value changed during throttle period, send final value if (_lastReportedValue != null && _lastReportedValue != widget.value) { widget.onChanged?.call(_lastReportedValue!); } }); } else { _lastReportedValue = newValue; } } } // Don't forget to cancel the timer in dispose() @override void dispose() { _throttleTimer?.cancel(); // Rest of existing code... }packages/naked/example/pubspec.yaml (2)
40-41: Remove trailing whitespaceThere are trailing spaces at the end of these lines that should be removed.
- cupertino_icons: ^1.0.6 - + cupertino_icons: ^1.0.6 + - url_strategy: ^0.2.0 - + url_strategy: ^0.2.0 + - flutter_svg: ^2.0.10 - + flutter_svg: ^2.0.10 +Also applies to: 43-44, 49-50
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 41-41: trailing spaces
(trailing-spaces)
80-109: Consider documenting required assets for the exampleWhile the commented sections provide guidance, it may be helpful to explicitly document any required assets that contributors should add when working with the example.
packages/naked/example/lib/examples/naked_menu_example.dart (2)
267-271: Consider making divider thickness consistentThe divider in the menu uses a color of
Colors.grey.shade200and height of 1, but the dividers between sections (lines 91 and 108) usethickness: 1instead. Consider making this consistent.Widget _buildDivider() { return Container( height: 1, color: Colors.grey.shade200, margin: const EdgeInsets.symmetric(vertical: 4), ); }
25-25: Fix unused submenu state variableThe
_isSubmenuOpenvariable is defined but never used in the main class, as the nested menu example handles its own submenu state.- final bool _isSubmenuOpen = false;packages/naked/lib/src/naked_menu.dart (2)
195-195: ParametercloseOnClickOutsideis never used
This attribute is declared but not implemented in the menu logic. Either remove it or implement the outside-tap handler to close the menu when enabled.- /// Whether to close the menu when clicked outside. - final bool closeOnClickOutside; + /// Whether to close the menu when clicked outside. + /// TODO: Implement outside click detection in the overlay background
473-474: Remove or integrate thefocusNodeparameter
ThefocusNodedeclared here is never used withinNakedMenuContent. Consider removing it or applying it to theFocuswidget if you intend to manage focus at the content level.packages/naked/example/lib/examples/naked_select_example.dart (3)
228-291: Consider factoring out the duplicate_buildTriggerVisualmethod.
This function is quite similar to the one in theFormSelectExample(lines 514-570). Consolidating these shared visual and styling behaviors into a single reusable method or separate widget can improve maintainability and avoid code drift.
293-372: Consider lazy-loading approaches for large option sets.
When handling very large lists in_buildMenuVisual, building all option widgets at once may become expensive. Utilizing lazy-loading or pagination could improve performance in scenarios where the options list grows significantly.
514-570: Avoid duplicating trigger rendering logic.
This_buildTriggerVisualmethod replicates much of the styling and layout code from the one in_NakedSelectExampleState. Centralizing these methods would help keep styling and behavior consistent.packages/naked/lib/src/naked_tabs.dart (2)
269-288: Be cautious about focus trapping in nested widgets.
EnablingtrapFocus: trueandrestoreFocus: trueis helpful for an isolated tabs component. However, nesting multipleNakedFocusManagers with focus trapping can lead to surprising navigation constraints. Consider making this behavior optional to reduce potential conflicts in complex layouts.
451-457: Consider making auto-focus on selected tabs configurable.
Requesting focus indidChangeDependencieswhen the tab is selected can unexpectedly steal the user’s focus—especially for mouse input users. Offering a toggle or condition for auto-focusing selected tabs helps avoid jarring experiences.packages/naked/example/lib/examples/naked_tabs_example.dart (2)
112-241: Consider reducing repetitive interaction state logic.Across
_BasicTabsState(and similarly in other classes) you maintain_hoverStates,_focusStates, and_pressStatesfor each tab. This pattern is repeated in multiple tab examples and may be consolidated into a shared helper or a higher-level custom widget that manages hover/focus/press states. Doing so can reduce code duplication while preserving clarity.
1009-1272: Watch out for potential performance impact in large product grids.The
_ResponsiveTabsGridexample usesGridView.builderinside aSingleChildScrollView, which can be fine for small datasets but might cause performance issues if the list of products grows significantly. Consider a lazy-loading approach or a list virtualization mechanism for large-scale data scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
packages/naked/README.md(1 hunks)packages/naked/analysis_options.yaml(1 hunks)packages/naked/docs/component_guide/README.md(1 hunks)packages/naked/docs/component_guide/state_callbacks.md(1 hunks)packages/naked/example/lib/examples/naked_accordion_example.dart(1 hunks)packages/naked/example/lib/examples/naked_button_example.dart(1 hunks)packages/naked/example/lib/examples/naked_checkbox_example.dart(1 hunks)packages/naked/example/lib/examples/naked_menu_example.dart(1 hunks)packages/naked/example/lib/examples/naked_radio_example.dart(1 hunks)packages/naked/example/lib/examples/naked_select_enhanced_example.dart(1 hunks)packages/naked/example/lib/examples/naked_select_example.dart(1 hunks)packages/naked/example/lib/examples/naked_tabs_example.dart(1 hunks)packages/naked/example/lib/main.dart(1 hunks)packages/naked/example/pubspec.yaml(1 hunks)packages/naked/lib/src/naked_accordion.dart(1 hunks)packages/naked/lib/src/naked_avatar.dart(1 hunks)packages/naked/lib/src/naked_button.dart(1 hunks)packages/naked/lib/src/naked_checkbox.dart(1 hunks)packages/naked/lib/src/naked_menu.dart(1 hunks)packages/naked/lib/src/naked_radio_button.dart(1 hunks)packages/naked/lib/src/naked_select.dart(1 hunks)packages/naked/lib/src/naked_slider.dart(1 hunks)packages/naked/lib/src/naked_tabs.dart(1 hunks)packages/naked/test/naked_avatar_test.dart(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/naked/docs/component_guide/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/naked/analysis_options.yaml
- packages/naked/test/naked_avatar_test.dart
- packages/naked/example/lib/examples/naked_select_enhanced_example.dart
- packages/naked/lib/src/naked_radio_button.dart
🧰 Additional context used
🪛 YAMLlint (1.35.1)
packages/naked/example/pubspec.yaml
[error] 41-41: trailing spaces
(trailing-spaces)
[error] 44-44: trailing spaces
(trailing-spaces)
[error] 50-50: trailing spaces
(trailing-spaces)
🔇 Additional comments (53)
packages/naked/example/lib/examples/naked_accordion_example.dart (6)
4-23: Neat example entry point for demonstrations.The
NakedAccordionExamplewidget effectively sets up a scrollable container showcasing different accordion variations. No functional issues stand out.
78-94: Good reuse of a helper method.The
_buildSectionhelper method streamlines repetitive section building. This is a clean approach that enhances readability and maintainability.
226-338: Adaptive multiple expansions look consistent.Your
_ColoredAccordionusesNakedAccordionType.multiplecorrectly to allow simultaneous expansions, and the color-based design changes are straightforward.
340-457: Clean single-item expansion example.The
_BorderedAccordionproperly demonstrates how to track the currently expanded item for single-mode usage. No issues noted.
459-576: Concise approach to single-accordion grouping.The
_AccordionGroupensures only one item remains open by updating_activeItemon change. The logic is simple and matchesNakedAccordionType.single.
830-973: Helpful icon usage for item identification.The
_IconAccordionapproach with icons and color highlights is clear. It provides a nice illustration of mixing visuals with the default logic.packages/naked/lib/src/naked_accordion.dart (6)
3-10: Enum usage is straightforward.
NakedAccordionTypeclearly distinguishes single vs. multiple expansion modes. This is a simple, readable approach.
12-216: Robust overall accordion structure.The
NakedAccordionand_NakedAccordionStateeffectively manage expanded states. The logic for toggling items (lines 177–194) is clear and correctly accounts for disabled state.
218-255: Inherited widget integration is well-structured.
_NakedAccordionScopecleanly exposes essential state and actions to descendant widgets. TheupdateShouldNotifymethod properly checks relevant fields.
257-421: Flexible trigger design.
NakedAccordionTriggersupports custom icons, hover, pressed, and focus states—ideal for building different UI styles without imposing constraints. No major issues.
423-526: Smooth expansion animation management.
NakedAccordionContentuses aSizeTransitionfor expansion, which is standard and fluid. The optional callbacks and overrides (lines 490–492) are well-handled.
622-645: Scoped item state is properly exposed.
_NakedAccordionItemScopeensures child widgets can reference the item’s value/disabled state. Update notifications handle changed values accurately.packages/naked/lib/src/naked_slider.dart (8)
1-5: Exports look good for reusabilityThe export statement makes
NakedSliderStateavailable to consumers, allowing them to access slider state from child widgets.
7-14: Well-documented enum with clear purposeThe
SliderDirectionenum is properly documented with comments explaining each value's behavior.
16-114: Excellent documentation with comprehensive exampleThe class documentation does an excellent job explaining the component's purpose, behavior, and keyboard navigation support. The included example is thorough and demonstrates proper implementation.
218-252: Good implementation of InheritedWidget patternThe
NakedSliderStateclass correctly follows the InheritedWidget pattern with a properofmethod and implementation ofupdateShouldNotify.
254-274: Proper focus node managementThe code correctly creates and disposes of the focus node when needed, preventing memory leaks.
276-288: Robust value normalization logicThe
_normalizeValuemethod properly handles both bounds checking and discretization when divisions are specified.
344-404: Comprehensive keyboard navigation supportExcellent implementation of keyboard navigation with support for:
- Arrow keys for small adjustments
- Page Up/Down for larger steps
- Home/End for jumping to min/max
- RTL language support
- Shift key for larger steps
This is great for accessibility.
420-501: Well-structured widget tree with clear layeringThe widget structure uses a layered approach (Semantics → NakedFocusManager → Focus → MouseRegion → GestureDetector → NakedSliderState) where each layer has a specific responsibility. This clean separation aligns well with the "behavior-first" UI approach mentioned in the PR objectives.
packages/naked/example/pubspec.yaml (6)
1-5: LGTM! Clear package metadata and publication settingsThe package name, description, and publication configuration are well-defined. The
publish_to: 'none'correctly prevents accidental publication since this is an example app.
19-19: LGTM! Appropriate version numberVersion
1.0.0+1follows semantic versioning practices for the example app.
21-22: LGTM! Dart SDK constraintsThe Dart SDK constraint
'>=3.4.3 <4.0.0'ensures compatibility with the latest Dart version while preventing breaking changes from major version upgrades.
34-36: LGTM! Correct path dependency setupThe path dependency to the root package is properly configured, ensuring the example uses the local version of the
nakedpackage.
58-60: LGTM! Integration tests configurationThe integration_test SDK dependency is correctly configured with a clear explanatory comment.
72-78: LGTM! Proper Flutter configurationThe Flutter section correctly includes Material Design icons, which is essential for the UI components.
packages/naked/docs/component_guide/state_callbacks.md (6)
1-6: LGTM! Clear documentation heading and overviewThe document starts with a clear title and a concise overview of the state callback naming convention, which is essential for establishing consistent patterns throughout the library.
7-21: LGTM! Well-defined naming pattern with examplesThe callback naming pattern is clearly defined with the format
on{State}Stateand accompanied by practical examples that illustrate the convention.
22-39: LGTM! Comprehensive callback documentationThe documentation provides a well-structured table of common state callbacks along with specialized callbacks, helping developers understand the available options and their use cases.
40-74: LGTM! Practical usage exampleThe example effectively demonstrates how to implement state callbacks with the
NakedButtoncomponent, showing a complete implementation with visual feedback based on state changes.
76-85: LGTM! Clear explanation of state management approachThis section effectively communicates the philosophy behind Naked components' state management approach, emphasizing that components provide callbacks but don't manage state internally.
86-98: LGTM! Helpful migration guideThe migration section provides clear guidance for users updating from previous versions, with a well-structured table mapping old patterns to new ones.
packages/naked/lib/src/naked_button.dart (8)
1-3: LGTM! Appropriate importsThe imports are concise and appropriate, including necessary Flutter packages and the
NakedFocusManagerutility which will help with focus management.
5-58: LGTM! Excellent documentation with exampleThe class documentation is comprehensive and includes a practical usage example that demonstrates how to implement a custom button with state management. This is extremely helpful for developers who will be using this component.
59-130: LGTM! Well-designed API with thorough property documentationThe class properties are well-designed with clear purposes and excellent documentation. The callbacks follow the naming convention described in the state_callbacks.md document, ensuring consistency across the library.
131-149: LGTM! Clean constructor with sensible defaultsThe constructor is well-organized with required and optional parameters. Default values are sensible, like
enableHapticFeedback: trueandcursor: SystemMouseCursors.click.
151-164: LGTM! Proper handling of interactivityThe build method starts by determining if the button is interactive based on disabled state, loading state, and the presence of an onPressed callback, which is a good practice for conditional rendering.
165-172: LGTM! Excellent accessibility implementationThe Semantics widget provides comprehensive accessibility information, including button state, enabled status, custom label, and loading hint.
172-196: LGTM! Proper keyboard navigation supportThe implementation includes thorough keyboard support through the
Focuswidget, handling both key down and key up events for space and enter keys, which is essential for accessibility.
197-211: LGTM! Complete interaction handlingThe MouseRegion and GestureDetector components properly handle all interaction states (hover, press, tap) and appropriately trigger the corresponding callbacks.
packages/naked/example/lib/examples/naked_menu_example.dart (8)
1-2: LGTM! Appropriate importsThe imports are minimal and focused, including only Flutter Material and the Naked package.
4-35: LGTM! Well-structured state managementThe example maintains clear state variables for menu open state, last action, and visual feedback states (hover, focus, pressed) that follow the pattern established in the documentation.
55-88: LGTM! Proper implementation of NakedMenuThe NakedMenu component is used correctly with appropriate properties for controlling open state, position preferences, and offset. The callbacks for state changes are properly implemented.
126-163: LGTM! Visual implementation with state-based stylingThe menu button visual implementation properly uses state variables to create dynamic styling based on interaction states, demonstrating the intended use case of the callback pattern.
165-208: LGTM! Well-structured menu contentThe menu content implementation includes appropriate styling, scrolling for overflow content, and a clean organization of menu items.
210-263: LGTM! Reusable menu item implementationThe
_buildMenuItemVisualmethod creates a reusable pattern for menu items with appropriate callbacks and state-based styling, demonstrating how to use the NakedMenuItem component effectively.
274-452: LGTM! Comprehensive focus/keyboard exampleThe focus menu example demonstrates keyboard navigation best practices, providing visual feedback and state tracking for keyboard interactions, which is essential for accessibility.
454-747: LGTM! Advanced nested menu implementationThe nested menu example showcases how to implement more complex menu hierarchies, with proper state management for both main and submenu components.
packages/naked/lib/src/naked_checkbox.dart (1)
217-218: Revisit the use oftoggled: isIndeterminatefor tri-state semantics
For tri-state checkboxes, Flutter does not officially provide a dedicated “toggled” property to indicate an indeterminate/mixed state. Consider exploring alternative semantics or verifying that screen readers correctly interpret this logic.Could you confirm with Flutter’s accessibility guidelines whether using
toggled: isIndeterminatecorrectly communicates this tri-state to assistive technologies?packages/naked/example/lib/examples/naked_button_example.dart (1)
1-913: No major issues detected in these sections.
All usage ofNakedButtonand the state management for button states (hover, press, focus) seem consistent and well-organized.packages/naked/lib/src/naked_tabs.dart (1)
1-630: Overall tab implementation looks solid.
The keyboard navigation, focus handling, and orientation handling are cleanly structured. No major logic issues observed.packages/naked/example/lib/main.dart (1)
1-333: Looks good overall.The routing and navigation setup is clear, and the approach to updating the URL for direct linkability appears sound. No major issues found in this file.
packages/naked/example/lib/examples/naked_radio_example.dart (1)
466-571: Overall radio examples are well-structured.The variety of usage scenarios (basic, styled, form-integrated, disabled) provides a robust demonstration of Naked’s headless radio buttons. Implementation and state updates appear correct aside from the noted scoping repetition.
| final effectiveFocusNode = focusNode ?? FocusNode(); | ||
|
|
||
| // Gets the current state of the checkbox |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Store the focus node in state to avoid creating a new one on each build
This FocusNode is instantiated on every rebuild, which can lead to lost focus information and potential memory leaks if the parent widget rebuilds frequently. Consider using a stateful widget and disposing of the focus node when no longer needed.
-class NakedCheckbox extends StatelessWidget {
+class NakedCheckbox extends StatefulWidget {
...
}
+class _NakedCheckboxState extends State<NakedCheckbox> {
+ final FocusNode _internalFocusNode = FocusNode();
+ @override
+ void dispose() {
+ _internalFocusNode.dispose();
+ super.dispose();
+ }
@override
Widget build(BuildContext context) {
- final effectiveFocusNode = focusNode ?? FocusNode();
+ final effectiveFocusNode = widget.focusNode ?? _internalFocusNode;
...
}
}📝 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.
| final effectiveFocusNode = focusNode ?? FocusNode(); | |
| // Gets the current state of the checkbox | |
| class NakedCheckbox extends StatefulWidget { | |
| // Existing properties and constructor remain unchanged | |
| final FocusNode? focusNode; | |
| const NakedCheckbox({Key? key, this.focusNode}) : super(key: key); | |
| @override | |
| _NakedCheckboxState createState() => _NakedCheckboxState(); | |
| } | |
| class _NakedCheckboxState extends State<NakedCheckbox> { | |
| final FocusNode _internalFocusNode = FocusNode(); | |
| @override | |
| void dispose() { | |
| _internalFocusNode.dispose(); | |
| super.dispose(); | |
| } | |
| @override | |
| Widget build(BuildContext context) { | |
| final effectiveFocusNode = widget.focusNode ?? _internalFocusNode; | |
| // Gets the current state of the checkbox | |
| // ... Rest of the widget build implementation ... | |
| return Container(); // Replace with actual widget tree | |
| } | |
| } |
tilucasoli
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
packages/naked/example/lib/examples/naked_button_example.dart (1)
1450-1481: Avoid duplicating container logic across multiple components.Both
_buildCardContainerand_buildSectionTitleappear in multiple classes. Extracting these into a shared widget or utility function can uphold the DRY principle and simplify maintenance.
🧹 Nitpick comments (11)
melos.yaml (1)
6-7: Narrowed Example Package SpecificationThe entry on line 7 has been changed from a wildcard inclusion to the specific directory
packages/mix/example. This aligns with the PR objective of narrowing the scope of example packages, focusing on the mix package. Please confirm that this exclusion of other example directories is intentional; if there are other packages with usable examples, they should be added explicitly or reconsidered.packages/naked/pubspec.yaml (2)
1-4: Metadata Verification and VersioningThe metadata is clearly defined with the library's name, description, version, and repository URL. Please confirm that the version
"0.0.1-dev.0"is intended as a development pre-release version; if a stable release is planned, consider switching to"0.0.1". Also, ensure that the repository URL is current and correct, and remove or update the inline comment if it no longer provides value.
19-53: Flutter Section and Documentation CommentsThis section correctly sets
uses-material-design: trueand provides comprehensive commented guidelines for assets and fonts. The detailed guidance can be helpful for new contributors. However, to keep the file concise, consider removing or relocating overly detailed comments once the package structure is stabilized.packages/naked/lib/src/naked_avatar.dart (3)
74-84: Consider adding loading and error state handlingThe current implementation doesn't provide visual feedback during image loading or when an error occurs. While this aligns with the "naked" approach, it might be helpful to provide guidance in the documentation on how to handle these states.
You could consider demonstrating this in the example by showing how to implement loading and error states:
imageWidgetBuilder: (context, child) { return Stack( children: [ // Base placeholder or fallback Container( height: 80, width: 80, decoration: BoxDecoration( color: Colors.grey.shade300, shape: BoxShape.circle, ), child: Center(child: Text('AB')), ), // Image when loaded successfully if (child != null) child, ], ); }
77-81: BoxFit.cover may cut off important parts of avatar imagesWhile
BoxFit.coverensures the image fills the container, it can crop important parts of profile pictures. Consider making thefitproperty configurable.decoration: BoxDecoration( image: DecorationImage( image: image!, onError: onError, - fit: BoxFit.cover, + fit: fit ?? BoxFit.cover, ), ),And add a new optional parameter to the class:
/// Optional BoxFit for the image. Defaults to BoxFit.cover. final BoxFit? fit;
64-70: Consider adding frameBuilder and loadingBuilder parametersTo provide more flexibility for image loading states, consider adding
frameBuilderandloadingBuilderparameters similar to Flutter's Image widget. This would align with the library's goal of giving developers full control.You could add these parameters to the constructor and use them in an Image widget instead of DecorationImage:
const NakedAvatar({ super.key, this.image, required this.imageWidgetBuilder, this.onError, this.semanticLabel, + this.frameBuilder, + this.loadingBuilder, }); // Add these properties final ImageFrameBuilder? frameBuilder; final ImageLoadingBuilder? loadingBuilder;packages/naked/lib/src/naked_button.dart (1)
202-209: Consider adding long-press gesture support.The button implements tap gestures but doesn't include support for long-press, which could be useful for certain button types (e.g., context menus, secondary actions).
child: GestureDetector( behavior: HitTestBehavior.opaque, onTapDown: isInteractive ? (_) => onPressedState?.call(true) : null, onTapUp: isInteractive ? (_) => onPressedState?.call(false) : null, onTapCancel: isInteractive ? () => onPressedState?.call(false) : null, onTap: isInteractive ? handleTap : null, + onLongPress: isInteractive ? widget.onLongPress : null, + onLongPressStart: isInteractive ? (_) => onPressedState?.call(true) : null, + onLongPressEnd: isInteractive ? (_) => onPressedState?.call(false) : null, child: child, ),packages/naked/example/lib/examples/naked_button_example.dart (1)
986-994: Consider adding proper error handling for asynchronous operations.The loading spinner simulation doesn't properly handle errors that might occur during the async operation, and it checks for
mountedbut doesn't have a complete error handling strategy.Enhance the error handling to provide better user feedback:
Future<void> handleSpinnerClick() async { if (_isSpinnerLoading) return; setState(() => _isSpinnerLoading = true); widget.onButtonPressed('Loading Spinner Started'); + try { // Simulate loading await Future.delayed(const Duration(seconds: 2)); if (mounted) { setState(() => _isSpinnerLoading = false); widget.onButtonPressed('Loading Spinner Completed'); } + } catch (e) { + if (mounted) { + setState(() => _isSpinnerLoading = false); + widget.onButtonPressed('Loading Spinner Failed: ${e.toString()}'); + } + } }packages/naked/naked_component_guide.md (3)
214-230: Missing "the" article in property naming conventions section.In the "Why prefer positive names without 'is' prefix?" section, there's a minor grammatical issue.
- **Readability**: In named parameters, omitting verbs creates more readable call sites + **Readability**: In the named parameters, omitting verbs creates more readable call sites🧰 Tools
🪛 LanguageTool
[uncategorized] ~221-~221: You might be missing the article “the” here.
Context: ... Why prefer positive names without "is" prefix? - Readability: In n...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
662-663: Word repetition in state management best practices.There's a word repetition in the state management best practices section.
- 1. **Consumer Responsibility**: Let consumers manage their own state + 1. **Consumer Responsibility**: Let consumers manage their state🧰 Tools
🪛 LanguageTool
[duplication] ~663-~663: Possible typo: you repeated a word.
Context: ...ility**: Let consumers manage their own state 2. State Variables: Use explicit state variabl...(ENGLISH_WORD_REPEAT_RULE)
237-263: Missing "onPress" callback in individual component state example.The example for individual component state includes
onPressedbut doesn't update it in the state callbacks section.@override Widget build(BuildContext context) { return NakedButton( onPressed: () { // Handle press }, - onStateHover: (isHovered) => setState(() => _isHovered = isHovered), - onStatePressed: (isPressed) => setState(() => _isPressed = isPressed), - onStateFocus: (isFocused) => setState(() => _isFocused = isFocused), + onHoverState: (isHovered) => setState(() => _isHovered = isHovered), + onPressedState: (isPressed) => setState(() => _isPressed = isPressed), + onFocusState: (isFocused) => setState(() => _isFocused = isFocused), child: Container( // Styling based on state variables ), ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
melos.yaml(1 hunks)packages/naked/example/.metadata(1 hunks)packages/naked/example/lib/examples/naked_avatar_example.dart(1 hunks)packages/naked/example/lib/examples/naked_button_example.dart(1 hunks)packages/naked/lib/src/naked_avatar.dart(1 hunks)packages/naked/lib/src/naked_button.dart(1 hunks)packages/naked/naked_component_guide.md(1 hunks)packages/naked/pubspec.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/naked/example/.metadata
- packages/naked/example/lib/examples/naked_avatar_example.dart
🧰 Additional context used
🪛 LanguageTool
packages/naked/naked_component_guide.md
[uncategorized] ~221-~221: You might be missing the article “the” here.
Context: ... Why prefer positive names without "is" prefix? - Readability: In n...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[duplication] ~663-~663: Possible typo: you repeated a word.
Context: ...ility**: Let consumers manage their own state 2. State Variables: Use explicit state variabl...
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test
- GitHub Check: Test Min SDK
- GitHub Check: production
🔇 Additional comments (11)
packages/naked/pubspec.yaml (3)
6-9: Environment Constraints ConfigurationThe SDK and Flutter version constraints are appropriately set with
"sdk: ">=3.6.0 <4.0.0"and"flutter: ">=3.27.0". These constraints appear valid given your intended support. It would be good to double-check if any dependencies require a newer SDK version.
10-13: Dependencies SetupThe dependencies section correctly lists Flutter as a dependency using the SDK reference. This standard configuration is well-formed and needs no changes.
14-18: Dev Dependencies SetupThe development dependencies, including
flutter_testandflutter_lints: ^4.0.0, are standard for Flutter package development. Ensure that theflutter_lintsversion aligns with your linting and style guidelines.packages/naked/lib/src/naked_avatar.dart (4)
1-45: Well-documented component with clear purpose and usage examplesThe documentation clearly explains the component's purpose and provides a comprehensive example. This aligns well with the library's "behavior-first" philosophy by explicitly stating it has no default styling.
46-47: Good use of typedef for the builder patternThe
ImageWidgetBuildertypedef clearly defines the contract for customizing the avatar's appearance, which is essential for the library's design flexibility goal.
49-63: Props and accessibility considerations look goodThe class includes proper documentation for each property and accessibility support through the semantic label. This aligns with the library's built-in accessibility features objective.
86-93: Good accessibility implementation with SemanticsUsing the
Semanticswidget with appropriate properties enhances accessibility, which is a key feature mentioned in the PR objectives.packages/naked/lib/src/naked_button.dart (2)
65-65: Well-structured child property with clear documentation.The clear documentation for the
childproperty correctly emphasizes that styling responsibilities lie with the consumer, which reinforces the "behavior-first" approach of the Naked library.
152-216: Well-implemented layered architecture following accessibility best practices.The button implementation follows the layered architecture described in the component guide, providing a proper hierarchy of:
- Semantics (accessibility)
- Focus management
- Mouse interaction
- Gesture handling
- Child rendering
The implementation correctly handles keyboard accessibility (Space/Enter keys), cursor management, and disabled states. The use of
NakedFocusManagerfor focus management is also appropriate.packages/naked/naked_component_guide.md (2)
9-27: Comprehensive guide with excellent structure.The guide provides a clear, thorough explanation of the Naked component philosophy, architecture, and implementation patterns. The table of contents and organization make it easy to navigate, and the explanations effectively communicate the "behavior-first" approach.
652-680: Well-defined best practices section.The best practices section provides valuable guidance on component design, state management, accessibility, and testing, which will help developers create consistent, high-quality components using the Naked library.
🧰 Tools
🪛 LanguageTool
[duplication] ~663-~663: Possible typo: you repeated a word.
Context: ...ility**: Let consumers manage their own state 2. State Variables: Use explicit state variabl...(ENGLISH_WORD_REPEAT_RULE)
| /// Use this to update the visual appearance when the user hovers over the button. | ||
| final ValueChanged<bool>? onHoverState; | ||
|
|
||
| /// Called when pressed state changes. | ||
| /// | ||
| /// Provides the current pressed state (true when pressed, false otherwise). | ||
| /// Use this to update the visual appearance when the user presses the button. | ||
| final ValueChanged<bool>? onPressedState; | ||
|
|
||
| /// Called when focus state changes. | ||
| /// | ||
| /// Provides the current focus state (true when focused, false otherwise). | ||
| /// Use this to update the visual appearance when the button receives or loses focus. | ||
| final ValueChanged<bool>? onFocusState; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
API naming inconsistency with component guide.
There's an inconsistency between the callback naming in this implementation and what's documented in the component guide. The guide uses onStateHover, onStateFocus, etc., while the implementation uses onHoverState, onFocusState, etc.
Consider aligning the naming convention across all components and documentation to ensure a consistent API. Which pattern should be the standard?
🏁 Script executed:
#!/bin/bash
# Check if other components follow the same naming pattern
rg "onHoverState|onPressedState|onFocusState" --type dartLength of output: 33849
Update Component Guide Documentation to Match Current API Naming
After verifying the codebase, it's clear that the implementation consistently uses the callback names:
onHoverStateonPressedStateonFocusState
This pattern appears across multiple components (e.g., in naked_button.dart, naked_accordion.dart, naked_tabs.dart, etc.). The only discrepancy is with the component guide documentation, which mentions names like onStateHover and onStateFocus.
Recommendation:
Update the component guide to reflect the current naming convention (onHoverState, onPressedState, and onFocusState) so that documentation and implementation are aligned.
|
|
||
| Widget _buildSubsectionTitle(String title) { | ||
| return Padding( | ||
| padding: const EdgeInsets.only(bottom: 12.0), | ||
| child: Text( | ||
| title, | ||
| style: TextStyle( | ||
| fontSize: 16, | ||
| fontWeight: FontWeight.w500, | ||
| color: Colors.grey[700], | ||
| ), | ||
| ), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Multiple implementations of the same helper methods across classes.
The _buildSubsectionTitle and _buildCardContainer methods are implemented similarly in multiple classes (ButtonStates, ToggleButtons, LoadingButtons, StyledButtons). This creates maintenance challenges if these need to be updated.
Extract these to shared utility methods or a common base class:
// In a separate utilities file or at the top of this file
class ExampleCardUtils {
static Widget buildCardContainer({
required String title,
required Widget child,
EdgeInsets padding = const EdgeInsets.all(16),
}) {
return Container(
width: double.infinity,
padding: padding,
decoration: BoxDecoration(
color: Colors.white,
borderRadius: BorderRadius.circular(8),
boxShadow: [
BoxShadow(
color: Colors.black.withOpacity(0.05),
blurRadius: 2,
offset: const Offset(0, 1),
),
],
),
child: Column(
crossAxisAlignment: CrossAxisAlignment.start,
children: [
Text(
title,
style: TextStyle(
fontSize: 16,
fontWeight: FontWeight.w500,
color: Colors.grey[700],
),
),
const SizedBox(height: 12),
child,
],
),
);
}
static Widget buildSectionTitle(String title, {
EdgeInsets padding = const EdgeInsets.only(bottom: 12.0),
TextStyle? style,
}) {
return Padding(
padding: padding,
child: Text(
title,
style: style ?? TextStyle(
fontSize: 16,
fontWeight: FontWeight.w500,
color: Colors.grey[700],
),
),
);
}
}Also applies to: 946-947, 1192-1224
| final ValueChanged<bool>? onStateHover; | ||
| final ValueChanged<bool>? onStatePressed; | ||
| final ValueChanged<bool>? onStateFocus; | ||
|
|
There was a problem hiding this comment.
Same naming inconsistency in component structure example.
The component structure example also uses the onStateX naming convention, which differs from the implementation.
// State callbacks
- final ValueChanged<bool>? onStateHover;
- final ValueChanged<bool>? onStatePressed;
- final ValueChanged<bool>? onStateFocus;
+ final ValueChanged<bool>? onHoverState;
+ final ValueChanged<bool>? onPressedState;
+ final ValueChanged<bool>? onFocusState;📝 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.
| final ValueChanged<bool>? onStateHover; | |
| final ValueChanged<bool>? onStatePressed; | |
| final ValueChanged<bool>? onStateFocus; | |
| final ValueChanged<bool>? onHoverState; | |
| final ValueChanged<bool>? onPressedState; | |
| final ValueChanged<bool>? onFocusState; |
|
|
||
| ### Standard Callback Pattern | ||
|
|
||
| All Naked components follow this standard callback pattern: | ||
|
|
||
| ```dart | ||
| // Core functionality callback | ||
| onPressed: () => doSomething(), | ||
|
|
||
| // State callbacks | ||
| onStateHover: (isHovered) => setState(() => _isHovered = isHovered), | ||
| onStateFocus: (isFocused) => setState(() => _isFocused = isFocused), | ||
| onStatePressed: (isPressed) => setState(() => _isPressed = isPressed), | ||
| ``` |
There was a problem hiding this comment.
Naming inconsistency in state callbacks between documentation and implementation.
The guide uses onStateHover, onStateFocus, etc., but the actual implementation in naked_button.dart uses onHoverState, onFocusState, etc. This inconsistency could confuse developers.
Standardize the naming convention across all components and documentation. For example:
// Standard callback pattern
- onStateHover: (isHovered) => setState(() => _isHovered = isHovered),
- onStateFocus: (isFocused) => setState(() => _isFocused = isFocused),
- onStatePressed: (isPressed) => setState(() => _isPressed = isPressed),
+ onHoverState: (isHovered) => setState(() => _isHovered = isHovered),
+ onFocusState: (isFocused) => setState(() => _isFocused = isFocused),
+ onPressedState: (isPressed) => setState(() => _isPressed = isPressed),Or update the implementation to match the documentation.
📝 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.
| ### Standard Callback Pattern | |
| All Naked components follow this standard callback pattern: | |
| ```dart | |
| // Core functionality callback | |
| onPressed: () => doSomething(), | |
| // State callbacks | |
| onStateHover: (isHovered) => setState(() => _isHovered = isHovered), | |
| onStateFocus: (isFocused) => setState(() => _isFocused = isFocused), | |
| onStatePressed: (isPressed) => setState(() => _isPressed = isPressed), | |
| ``` | |
| ### Standard Callback Pattern | |
| All Naked components follow this standard callback pattern: | |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/naked/lib/src/naked_checkbox.dart (1)
184-185: 🛠️ Refactor suggestionPersist the
FocusNodein state to prevent potential memory leaks
This widget re-creates a newFocusNodeevery time the widget tree rebuilds, which can lead to lost focus context and unintentional memory usage growth if rebuilds occur frequently. Storing the node in a stateful widget and disposing of it when no longer needed is more robust.Here's a sketch of how you might refactor:
-class NakedCheckbox extends StatelessWidget { +class NakedCheckbox extends StatefulWidget { const NakedCheckbox({ ... }); + @override + State<NakedCheckbox> createState() => _NakedCheckboxState(); +} +class _NakedCheckboxState extends State<NakedCheckbox> { + final FocusNode _localFocusNode = FocusNode(); + @override + void dispose() { + _localFocusNode.dispose(); + super.dispose(); + } @override Widget build(BuildContext context) { - final effectiveFocusNode = focusNode ?? FocusNode(); + final effectiveFocusNode = widget.focusNode ?? _localFocusNode; ... } }packages/naked/example/lib/examples/naked_checkbox_example.dart (1)
315-424: Eliminate repetitive checkbox visual builder code
Multiple methods named_buildDefaultCheckboxVisualor_buildStyledCheckboxVisualcontain very similar logic. This duplication is noted in past reviews and can be consolidated into a shared utility or widget to embrace DRY principles.A possible approach:
// In a common utilities file, e.g., checkbox_visual_utilities.dart Widget buildCheckboxVisual({ required bool isChecked, bool isIndeterminate = false, bool isHovered = false, bool isPressed = false, bool isFocused = false, bool isDisabled = false, required Color color, ... }) { // Single implementation here }Also applies to: 597-676, 845-924, 1283-1362
🧹 Nitpick comments (3)
packages/naked/example/integration_test/naked_avatar_example_test.dart (3)
44-49: Consider mocking network image loadingThe test currently relies on actual network calls to load avatar images, which can lead to flaky tests if the network is unreliable or if the image URLs change.
- // Pump and settle to allow network images (including the erroring one) to resolve - await tester.pumpAndSettle(); + // Mock network image loading instead of making real network calls + // Example using NetworkImage.provider: + await precacheImage(NetworkImage('mock-url'), tester.element(find.byType(NakedAvatar).first)); + await tester.pump();
51-76: Consider adding keys to specific avatars for more straightforward testingThe finder for the avatar is quite complex. Adding keys to specific avatars would make the test more readable and maintainable.
- final initialBasicAvatarFinder = find - .descendant( - of: find.ancestor( - of: find.text('Basic Avatar'), matching: find.byType(Column)), - matching: find.byType(NakedAvatar), - ) - .first; + // With a key added to the basic avatar in the example widget: + final initialBasicAvatarFinder = find.byKey(const ValueKey('basic_avatar'));
1-94: Consider adding tests for edge casesWhile the current tests verify basic functionality, consider adding tests for edge cases such as:
- Image loading errors
- Empty or invalid URLs
- Different avatar sizes
- Callback invocation if NakedAvatar supports callbacks (onTap, onError, etc.)
This would provide more comprehensive test coverage for the component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/naked/example/integration_test/naked_avatar_example_test.dart(1 hunks)packages/naked/example/lib/examples/naked_checkbox_example.dart(1 hunks)packages/naked/lib/src/naked_checkbox.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test Min SDK
- GitHub Check: Test
- GitHub Check: production
🔇 Additional comments (6)
packages/naked/example/integration_test/naked_avatar_example_test.dart (4)
1-5: All necessary imports are presentThe imports are correctly set up for a Flutter integration test, importing the example widget, Flutter Material, test utilities, and the Naked library.
6-8: Good use of a constant key for widget identificationUsing a ValueKey constant for the refresh button is a good practice as it provides a stable identifier for finding widgets in tests.
9-43: Test structure follows best practicesThe first test properly sets up the widget in a MaterialApp context and methodically verifies the presence of all expected UI elements. The hierarchical checks using
find.descendantare a good practice for verifying nested elements.
77-92: Test effectively verifies URL change on refreshThe test correctly validates that tapping the refresh button results in a new avatar image by comparing URLs. The split check at the end helps ensure a meaningful change rather than just timestamp differences.
packages/naked/lib/src/naked_checkbox.dart (1)
216-221:✅ Verification successful
Verify semantics usage for tri-state checkboxes
Usingchecked: checkedtogether withtoggled: indeterminatemight not accurately convey tri-state semantics to assistive technologies. Verify with screen readers or official Flutter docs that this approach communicates the indeterminate state correctly.
🏁 Script executed:
#!/bin/bash # Script to locate any references to Semantics usage in tri-state patterns and gather context. rg -A 5 -i "Semantics"Length of output: 56805
Tri-State Semantics Implementation Verified
After reviewing the code and tests (especially the checks in
packages/naked/test/src/naked_checkbox_test.dartverifying that the indeterminate state correctly sets both thehasToggledStateandisToggledflags), the current approach usingreturn Semantics( checked: checked, toggled: indeterminate, enabled: isInteractive, label: semanticLabel, onTap: isInteractive ? toggleValue : null, … )confirms that the tri-state checkbox semantics are properly implemented. The tests ensure that assistive technologies will receive the correct indications for both checked and indeterminate states. While it is still beneficial to verify these behaviors with actual screen readers when possible, no changes are needed to the current implementation.
packages/naked/example/lib/examples/naked_checkbox_example.dart (1)
1364-1625: Focus and keyboard navigation logic looks solid
This well-structured approach leveragesFocusTraversalGroupand integrates hover/press/focus states effectively. No concerns here.
|
I’m curious about the advantages and disadvantages of adopting a const buttonBold = Button(
icon: Icon(Icons.format_bold),
label: Text('Bold'),
style: ButtonStyle(
backgroundColor: ItemStatePropertyAll<Color?>(Color(0xFFBBDEFB)),
),
);
final buttonUnderline = Button(
icon: const Icon(Icons.format_underline),
label: const Text('Underline'),
style: const ButtonStyle(
backgroundColor: ItemStatePropertyValues<Color?>(
defaultValue: Color(0xFFBBDEFB),
disabledValue: Color(0xFFE0E0E0),
selectedValue: Color(0xFFBBDEFB),
hoveredValue: Color(0xFF90CAF9),
pressedValue: Color(0xFF64B5F6),
),
),
onPress: () => print('Pressed'),
onLongPress: () => print('Long Pressed'),
); |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/naked/example/lib/examples/naked_menu_example.dart (1)
1-797:⚠️ Potential issueCommented-out example code needs attention.
The entire file contains commented-out code. This appears to be a work-in-progress example that demonstrates NakedMenu functionality but is not currently active. Consider either:
- Uncommenting and completing the example if it's ready for use
- Adding a TODO comment explaining why the code is commented out and when it will be completed
- Removing the file if it's not needed
Having large blocks of commented code can make the codebase harder to maintain.
♻️ Duplicate comments (1)
packages/naked/lib/src/naked_select.dart (1)
285-288: Dispose item focus nodes to prevent memory leaks.This code simply clears
_itemFocusNodeswithout disposing them. As noted in a previous comment,FocusNodeobjects should be disposed when they are no longer needed. Keep in mind that this helps prevent memory leaks in long-lived widgets.void _clearRegisteredValues() { + for (final node in _itemFocusNodes) { + node.dispose(); + } _itemFocusNodes.clear(); _selectableValues.clear(); }
🧹 Nitpick comments (5)
packages/naked/example/lib/examples/naked_tooltip_example.dart (1)
13-16: Unused controller in parent state.The controller in
_NakedTooltipExampleStateis initialized but never used. Consider removing it or utilizing it in the component.- Timer? _hoverTimer; - final controller = OverlayPortalController(); + Timer? _hoverTimer;packages/naked/lib/naked.dart (1)
36-39: Consider removing commented-out code.The commented-out export statements could be confusing for developers. If they're no longer needed, consider removing them entirely rather than keeping them as comments.
- // Core - removed core export as we're now just using common directly - // export 'src/core/core.dart'; - // export 'src/slider/headless_slider.dart' - // hide InteractiveStateSetStringExtension;packages/naked/lib/src/naked_select.dart (1)
195-197: Consider storing values and focus nodes in a unified structure.Currently,
_selectableValuesand_itemFocusNodesare maintained in parallel lists. This can lead to indexing errors, complicate synchronization, and hamper future maintainability. Think about consolidating them into a single collection to keep each item’s data and focus node together.Also applies to: 278-283
packages/naked/example/lib/main.dart (1)
84-101: Offer a distinct route for unknown paths.You currently default to loading index 0 if a path does not match. While this is acceptable for many use cases, providing a dedicated error or “not found” route could improve user guidance and maintainability, especially as the application grows.
packages/naked/lib/src/naked_tooltip.dart (1)
140-141: Remove unused mixin or implement actual animation usage.
_NakedTooltipStatemixes inSingleTickerProviderStateMixinbut does not use anAnimationController. Either remove the mixin if unneeded, or implement your animation logic to justify the mixin’s presence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/naked/example/integration_test/app_navigation_test.dart(1 hunks)packages/naked/example/lib/examples/naked_menu_example.dart(1 hunks)packages/naked/example/lib/examples/naked_select_enhanced_example.dart(1 hunks)packages/naked/example/lib/examples/naked_select_example.dart(1 hunks)packages/naked/example/lib/examples/naked_tooltip_example.dart(1 hunks)packages/naked/example/lib/main.dart(1 hunks)packages/naked/lib/naked.dart(1 hunks)packages/naked/lib/src/naked_menu.dart(1 hunks)packages/naked/lib/src/naked_select.dart(1 hunks)packages/naked/lib/src/naked_tooltip.dart(1 hunks)packages/naked/lib/src/naked_widgets.dart(1 hunks)packages/naked/test/naked_menu_test.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/naked/example/integration_test/app_navigation_test.dart
🔇 Additional comments (13)
packages/naked/lib/src/naked_widgets.dart (1)
1-10: Well-organized component exports.This file follows the standard Dart pattern for exposing a library's components, making it easy for developers to access all Naked UI components through a single import.
packages/naked/example/lib/examples/naked_tooltip_example.dart (4)
13-21: Proper resource cleanup in the dispose method.The state management and cleanup are handled correctly by canceling the timer in dispose, which prevents memory leaks and potential issues with timers firing after widget disposal.
45-68: Good implementation of animation for tooltip.The implementation uses SingleTickerProviderStateMixin and proper animation setup with CurvedAnimation. The animation controller is correctly initialized and disposed of, following Flutter best practices.
72-97: Well-structured NakedTooltip implementation with flexible positioning.The tooltip configuration is comprehensive, with:
- Fallback alignments for responsive positioning
- Custom anchor points
- Offset control for fine-tuning positioning
- Animation integration through FadeTransition
This demonstrates the flexibility of the NakedTooltip component.
98-111: Effective hover handling with delayed tooltip display.The MouseRegion implementation correctly handles mouse enter/exit events with timer-based delayed tooltip display. The 2-second delay before showing the tooltip follows common UX patterns, and the animation reversing on exit creates a smooth dismissal experience.
packages/naked/lib/naked.dart (3)
1-14: Excellent documentation of the library's design philosophy.The documentation clearly explains the Naked library's callback-driven approach and its three key principles:
- Components provide callbacks for state changes
- Consumers manage their own state
- Consumers have full control over styling
This sets clear expectations for library users and aligns with the behavior-first UI component approach described in the PR objectives.
16-27: Complete export of all Naked components.All Naked components are exported in a structured manner, making them easily accessible to consumers. The organization follows a clean pattern, and the export of the newly added NakedTooltip component is correctly included.
31-34: Clear roadmap for future components.The comments indicating future components (NakedToggle, NakedCombobox, NakedDialog) provide a useful roadmap for developers and users of the library. This transparency helps users understand what's coming next.
packages/naked/example/lib/examples/naked_select_enhanced_example.dart (1)
1-289: Confirm the intent of fully commented-out code.
It appears that the entire file is commented out. If this is meant to be functional example code, please consider uncommenting so that it can compile and run. Otherwise, to avoid confusion, remove or clarify the commented sections if they are no longer needed.packages/naked/test/naked_menu_test.dart (1)
1-250: Verify necessity of commented-out tests.
All lines in this new test file are commented out. If these tests are intended to run, please uncomment them so they can be executed. If they are placeholders or examples, consider removing or documenting them explicitly to maintain clarity.packages/naked/example/lib/examples/naked_select_example.dart (1)
1-957: Decide whether to keep or remove fully commented-out code.
This file contains extensive examples, but all lines are commented out. If these are intended as runnable examples, please uncomment and verify they function as expected. If they serve as reference documentation, consider adding clarifying comments to distinguish intentionally inactive code.packages/naked/lib/src/naked_menu.dart (1)
1-597: Assess the purpose of the commented-out implementation.
The entire menu component is currently inaccessible as all lines are commented out. Verify whether you meant to disable this code temporarily or if it should be active in production. If the logic is ready, please uncomment for proper usage; otherwise, add a note explaining the placeholder status.packages/naked/lib/src/naked_tooltip.dart (1)
143-146:❓ Verification inconclusive
Verify null-safety of retrieving the Overlay instance.
Overlay.of(context)can return null if noOverlayis present. Though most Material apps provide an overlay, consider safeguarding against a potential null, especially for test environments or custom widget trees.
🏁 Script executed:
#!/bin/bash # Description: Search for any custom usage of 'NakedTooltip' outside of a MaterialApp or Navigator. ast-grep --pattern $'NakedTooltip($_)' --jsonLength of output: 49
Ensure Null-Safety for Overlay Retrieval
The analysis confirms that our usage of
NakedTooltipisn’t found outside typical contexts (i.e., within a MaterialApp/Navigator), so an Overlay is almost always present. However, to further safeguard against potential null returns in custom widget trees or test environments, it’s recommended to explicitly handle anullOverlay if that scenario arises.
- File: packages/naked/lib/src/naked_tooltip.dart, Lines: 143-146
- Suggestion: Consider revising the code to either assert non-null or provide a fallback. For example:
final OverlayState? overlayState = Overlay.of(context, debugRequiredFor: widget); if (overlayState == null) { // Handle the absence of an Overlay (e.g., provide an early return or error message) }
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/naked/lib/src/naked_menu.dart (1)
259-270: Proper implementation of outside click detectionThe
_handlePointerDownmethod correctly respects thecloseOnClickOutsideproperty before registering global pointer routes. This ensures the component behavior aligns with the API specification.
🧹 Nitpick comments (3)
packages/naked/lib/src/utilities/naked_portal.dart (2)
111-114: Consider additional relayout triggersThe
shouldRelayoutmethod only checks for changes in target position and size, but doesn't consider changes in alignment configurations.Consider updating the
shouldRelayoutmethod to also check for changes in alignment configurations:@override bool shouldRelayout(_NakedPositionDelegate oldDelegate) { - return target != oldDelegate.target || targetSize != oldDelegate.targetSize; + return target != oldDelegate.target || + targetSize != oldDelegate.targetSize || + alignment != oldDelegate.alignment; }
164-173: Consider safe area insets for visibility checkThe visibility check ensures the overlay fits within the screen bounds, but doesn't account for safe areas or device notches.
Consider enhancing the visibility check to account for safe areas by passing MediaQuery insets:
bool _isOverlayFullyVisible( Offset overlayTopLeft, Size overlaySize, Size screenSize, + EdgeInsets safeAreaInsets = EdgeInsets.zero, ) { return overlayTopLeft.dx >= safeAreaInsets.left && overlayTopLeft.dy >= safeAreaInsets.top && overlayTopLeft.dx + overlaySize.width <= screenSize.width - safeAreaInsets.right && overlayTopLeft.dy + overlaySize.height <= screenSize.height - safeAreaInsets.bottom; }packages/naked/example/lib/examples/naked_menu_example.dart (1)
813-813: Fix whitespace in codeThere's extra whitespace between
_hoverStatesand[label]which should be removed for consistency.- onFocusState: (focus) => setState(() => _hoverStates [label] = focus), + onFocusState: (focus) => setState(() => _hoverStates[label] = focus),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/naked/example/lib/examples/naked_menu_example.dart(1 hunks)packages/naked/example/lib/main.dart(1 hunks)packages/naked/lib/src/naked_menu.dart(1 hunks)packages/naked/lib/src/naked_tooltip.dart(1 hunks)packages/naked/lib/src/utilities/naked_portal.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/naked/lib/src/naked_tooltip.dart
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test Min SDK
- GitHub Check: Test
- GitHub Check: production
🔇 Additional comments (22)
packages/naked/example/lib/main.dart (7)
1-14: Imports are well-organizedThe imports are properly organized with a clear separation between framework imports, external packages, and local examples. Good practice to group related imports together.
16-20: Good initialization patternProperly initializes URL strategy before running the app, which is important for handling routes correctly across platforms. The separation of concerns is clear.
28-44: Theme configuration is well-definedThe theme is thoroughly configured with proper use of Material 3, semantic color naming, and consistent styling. Using GoogleFonts with
interTextThemeis a good approach for font consistency.
85-101: Route handling is robustThe route generation logic properly parses paths and matches them to component examples. The fallback to initial index when a component isn't found prevents potential navigation errors.
152-155: Commented-out component in navigation listThere's a commented-out
NakedSelectcomponent in the destinations list, suggesting it might be in development but not ready for the example app.Is the
NakedSelectcomponent planned for a future release? Consider adding a TODO comment to clarify the status of this component.
193-200: Clean URL path update mechanismThe URL path update mechanism is implemented well, ensuring it doesn't add to the browser history stack. This provides a good user experience when navigating between examples.
246-273: Drawer navigation implementation is robustThe navigation drawer implementation is clean with good visual feedback for the selected item. The mapping of destinations to list tiles is efficient and maintains proper state tracking.
packages/naked/lib/src/utilities/naked_portal.dart (4)
3-24: Well-structured reusable overlay componentThe
NakedPortalwidget provides a clean API for positioning overlay content relative to a target. The default alignment configuration is sensible and the API provides flexibility through optional parameters.
26-60: Proper overlay positioning implementationThe
_NakedPortalStatecorrectly usesOverlayPortalandCustomSingleChildLayoutfor positioning. The use offindRenderObjectand coordinate transformation is appropriate for determining global positions.
62-72: Clean alignment abstractionThe
AlignmentPairclass provides a concise way to specify how content should be positioned relative to a target. The inclusion of an offset parameter adds flexibility for fine-tuning positioning.
117-147: Robust fallback positioning logicThe positioning logic properly attempts to place the overlay within screen bounds using the preferred alignment first, then falling back to alternatives if necessary. This is a good approach for handling different screen sizes and orientations.
packages/naked/lib/src/naked_menu.dart (6)
5-43: Excellent documentation with usage exampleThe class documentation is comprehensive with a clear explanation of the component's purpose and a practical usage example. This greatly enhances developer experience when using the library.
87-89: Commented-out trapFocus propertyThere's a commented-out property for trap focus functionality. This suggests a planned feature that's not yet implemented.
Is the
trapFocusfunctionality planned for a future release? Consider adding a TODO comment to clarify the status of this feature or removing the commented code if it's not planned.
140-155: Proper focus and overlay managementThe state class correctly initializes and disposes of the focus scope node, which is important for preventing memory leaks and ensuring proper focus behavior throughout the component's lifecycle.
156-174: Effective state update handlingThe
didUpdateWidgetandhandleOpenmethods ensure that the overlay and focus state are updated correctly when the open state changes. The use ofaddPostFrameCallbackis appropriate to ensure UI updates happen after the frame is built.
191-201: Good keyboard event handlingThe escape key handling is properly implemented to close the menu. This is essential for keyboard accessibility and follows standard UI patterns.
374-394: Comprehensive focus and keyboard handlingThe
NakedMenuItemimplementation handles focus, keyboard events, and interaction states thoroughly. The support for space and enter keys for activation is important for accessibility.packages/naked/example/lib/examples/naked_menu_example.dart (5)
4-67: Well-structured example organizationThe main example component is well-organized with clear section headings and descriptions. The use of
SingleChildScrollViewensures the examples are accessible on smaller screens.
108-199: Clean basic menu implementationThe basic menu example demonstrates core functionality with proper state management and user feedback. The structure provides a good foundation for understanding how to implement a simple menu.
347-401: Effective keyboard navigation demonstrationThe keyboard navigation example effectively demonstrates how to track and respond to focus states. The visual indicators for keyboard focus enhance accessibility and provide important visual feedback.
459-507: Creative styling approachThe styled menu example showcases how to create visually rich components using the unstyled base. The gradient button and state-based animations demonstrate the flexibility of the headless approach.
681-785: Comprehensive complex menu exampleThe multi-section menu example effectively demonstrates how to handle larger, more complex menu structures with proper scrolling support. This is an important pattern for real-world applications.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
packages/naked/lib/src/naked_radio_group.dart (1)
89-125: 🛠️ Refactor suggestionRuntime type checking is fragile.
This has been brought up in previous reviews as well. Depending onruntimeType.toString().contains('NakedRadioButton<$T>')can break when the class's name is minified, changed, or otherwise refactored. Prefer a more robust check, such aselement.widget is NakedRadioButton<T>.- if (element.widget.runtimeType.toString().contains('NakedRadioButton<$T>')) { + if (element.widget is NakedRadioButton<T>) {packages/naked/example/lib/examples/naked_radio_example.dart (3)
36-84:⚠️ Potential issueFix state management in the
_buildRadioOptionmethod.The local variables
isHoveredandisPressedare initialized outside theStatefulBuilderbut are updated within the callbacks. This creates a variable scoping issue where the callbacks are not correctly updating the variables used in the UI.Fix the variable scoping by moving them inside the builder function:
Widget _buildRadioOption(String value, String label) { - bool isHovered = false; - bool isPressed = false; return StatefulBuilder( builder: (context, setInnerState) { + bool isHovered = false; + bool isPressed = false; final isSelected = _selectedOption == value; return Row( children: [ NakedRadioButton<String>( value: value, onHoverState: (hovering) => setInnerState(() => isHovered = hovering), onPressedState: (pressing) => setInnerState(() => isPressed = pressing),
126-181:⚠️ Potential issueFix state management in the
_buildStyleOptionmethod.Similar to the previous issue, the variables
isHovered,isPressed, andisFocusedare declared outside theStatefulBuilderbut updated within the callbacks.Move the state variables inside the builder function:
Widget _buildStyleOption(String value, Color color, String label) { - bool isHovered = false; - bool isPressed = false; - bool isFocused = false; return StatefulBuilder( builder: (context, setInnerState) { + bool isHovered = false; + bool isPressed = false; + bool isFocused = false; final isSelected = _selectedOption == value; return Column( children: [ NakedRadioButton<String>( value: value, semanticLabel: label, onHoverState: (hovering) => setInnerState(() => isHovered = hovering), onPressedState: (pressing) => setInnerState(() => isPressed = pressing), onFocusState: (focusing) => setInnerState(() => isFocused = focusing),
289-344:⚠️ Potential issueFix state management in the
_buildPriorityOptionmethod.The same variable scoping issue appears here with
isHoveredandisPressedvariables.Move the state variables inside the builder function:
Widget _buildPriorityOption( FormFieldState<String> state, String value, String label, Color color) { - bool isHovered = false; - bool isPressed = false; return StatefulBuilder( builder: (context, setInnerState) { + bool isHovered = false; + bool isPressed = false; final isSelected = state.value == value; return Row( children: [ NakedRadioButton<String>( value: value, semanticLabel: label, onHoverState: (hovering) => setInnerState(() => isHovered = hovering), onPressedState: (pressing) => setInnerState(() => isPressed = pressing),
🧹 Nitpick comments (3)
packages/naked/test/naked_radio_test.dart (1)
287-335: Test name vs. actual keyboard interaction.
The test is named "handles keyboard activation," yet it simulates taps rather than dispatching key events. To ensure actual keyboard handling is correct, you might want to dispatchKeyDownEventfor space or enter.packages/naked/lib/src/naked_radio_group.dart (1)
60-87: Post-frame callback usage.
Collecting radio button values after the frame ensures correct traversal, but keep in mind that frequent calls to_collectRadioButtonValuescan trigger multiple post-frame callbacks. If performance becomes an issue in large trees, consider deferring or batching.packages/naked/example/lib/examples/naked_radio_example.dart (1)
51-53: Add handling for focus state changes.The focus state callback is not actually updating any state, unlike hover and pressed callbacks.
Consider updating the focus state in the same way as other states:
onFocusState: (isFocused) => {}, +onFocusState: (isFocused) => + setInnerState(() => /* update focus state */),Or, if focus state tracking isn't needed in this example, add a comment explaining why.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/naked/example/lib/examples/naked_radio_example.dart(1 hunks)packages/naked/lib/src/naked_radio_button.dart(1 hunks)packages/naked/lib/src/naked_radio_group.dart(1 hunks)packages/naked/test/naked_radio_test.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test
- GitHub Check: Test Min SDK
- GitHub Check: production
🔇 Additional comments (22)
packages/naked/test/naked_radio_test.dart (6)
1-5: No issues with imports.
These imports are standard and appropriate for Flutter widget testing.
6-24: Test setup forNakedRadioGrouplooks good.
Therenders childscenario correctly verifies the presence of the wrapped child widget. No further concerns.
74-116: Consider verifying actual trapped focus.
While the test checks the presence ofNakedFocusManagerandKeyboardListener, it doesn’t assert that focus traversal is actually restricted to the group. If focus trapping is critical, consider adding a scenario that attempts to tab out of the group and verifies focus remains within it.
119-139: Straightforward child rendering test.
This test properly validates that a child widget is rendered when the radio button is part of the group.
141-185: Tap-to-selection change is correct.
This scenario ensures the callback updates theselectedValueupon tapping a different radio button. The logic and assertions appear correct.
232-286: Hover and pressed callback tests look good.
These event-driven tests provide strong coverage of pointer interactions, verifying hover and pressed states.packages/naked/lib/src/naked_radio_button.dart (7)
1-5: Imports are appropriate.
These imports for Flutter and local files are suitably minimal.
6-96: Comprehensive documentation.
The doc comments provide a clear example illustrating how to useNakedRadioButton. This is helpful for new consumers and fully aligns with the "behavior-first" philosophy.
97-155: Widget API design is consistent.
The constructor parameters and callback-based design are well-structured. Enforcing thechildandvalueparameters ensures the widget is purposeful.
157-172: Focus node disposal is handled properly.
The code carefully disposes of the local focus node only if the user didn’t supply an external one. This logic prevents memory leaks.
174-196: Haptic feedback is optional and non-blocking.
The_handleTapmethod nicely wraps optional haptic feedback. This gives developers freedom without incurring overhead if it’s not needed.
198-205: Keyboard event handling is straightforward.
Space or enter triggers_handleTap(). This is a typical Flutter pattern and helps ensure good accessibility for users navigating via keyboard.
207-263: Well-structured build method.
The layering ofSemantics,MouseRegion,Focus, andGestureDetectoris neatly organized, aligning with the Naked library’s concept of providing interaction logic without styling.packages/naked/lib/src/naked_radio_group.dart (6)
1-5: Minimal imports are suitable.
These imports are necessary for material, services, and local utilities.
6-58: API shape looks coherent.
Properties likevalue,onChanged,enabled,trapFocus,autofocus, andonEscapePressedalign well with a standard radio group. This gives consumers essential control over selection and focus logic.
127-145: Focus management approach is excellent.
Wrapping the child inNakedFocusManagerandKeyboardListenerensures a central point of control for radio navigation and escape handling.
147-162: Arrow key navigation logic is clear.
Using_moveToNextRadioButton(),_moveToPreviousRadioButton(),_moveToFirstRadioButton(), and_moveToLastRadioButton()is straightforward and predictable for end users.
164-214: Wrap-around navigation is a nice touch.
Allowing arrow down/right to loop to the first radio button (and arrow up/left to loop to the last) can be beneficial for certain UI scenarios, providing a cyclical experience.
218-250: Inherited widget usage.
NakedRadioGroupScopeis well-structured, enabling easy access to group state from child radio buttons. TheupdateShouldNotifymethod ensures accurate re-builds when relevant data changes.packages/naked/example/lib/examples/naked_radio_example.dart (3)
374-387: Verify the enabled property implementation in NakedRadioGroup.There appears to be a discrepancy in how disabled states are implemented. At line 380, the
enabledproperty is set totrue, but all child options are created withenabled: false(line 383, 385). This suggests that either the group is incorrectly enabled or the child options are incorrectly disabled.Please check whether this implementation matches the intended behavior. If the entire group should be disabled, change line 380 to
enabled: false.
1234-1343: Great focus and accessibility implementation!The
_buildFocusableRadioOptionmethod provides an excellent example of implementing proper focus management, hover states, and keyboard navigation. The use of state maps to track different states for each option is a more robust approach than the individual state variables used in other examples.
1114-1232: Excellent keyboard navigation and accessibility support.The
FocusRadioExampleclass does a great job demonstrating keyboard controls and making them visible to users with clear instructions. The implementation ofonEscapePressedfor clearing selection is a useful accessibility feature.
| testWidgets('does not respond when individually disabled', | ||
| (WidgetTester tester) async { | ||
| String? selectedValue = 'option1'; | ||
|
|
||
| // Create a group with buttons where one button is disabled | ||
| await tester.pumpWidget( | ||
| MaterialApp( | ||
| home: Scaffold( | ||
| body: StatefulBuilder( | ||
| builder: (context, setState) { | ||
| return NakedRadioGroup<String>( | ||
| value: selectedValue, | ||
| onChanged: (value) { | ||
| setState(() { | ||
| selectedValue = value; | ||
| }); | ||
| }, | ||
| child: const Column( | ||
| children: [ | ||
| NakedRadioButton<String>( | ||
| value: 'option1', | ||
| child: SizedBox(width: 20, height: 20), | ||
| ), | ||
| NakedRadioButton<String>( | ||
| value: 'option2', | ||
| enabled: true, | ||
| child: SizedBox(width: 20, height: 20), | ||
| ), | ||
| ], | ||
| ), | ||
| ); | ||
| }, | ||
| ), | ||
| ), | ||
| ), | ||
| ); | ||
|
|
||
| // Try to tap the disabled option | ||
| await tester.tap(find.byType(NakedRadioButton<String>).at(1)); | ||
| await tester.pump(); | ||
|
|
||
| // Verify selection did not change | ||
| expect(selectedValue, 'option1'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mismatch in disabled test parameters.
The test name indicates one button is disabled, but the code at line 211 sets enabled: true. This contradicts the stated goal of verifying no response on tapping a disabled button. Update the relevant line to enabled: false if you want to confirm disabled behavior.
- enabled: true,
+ enabled: false,📝 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.
| testWidgets('does not respond when individually disabled', | |
| (WidgetTester tester) async { | |
| String? selectedValue = 'option1'; | |
| // Create a group with buttons where one button is disabled | |
| await tester.pumpWidget( | |
| MaterialApp( | |
| home: Scaffold( | |
| body: StatefulBuilder( | |
| builder: (context, setState) { | |
| return NakedRadioGroup<String>( | |
| value: selectedValue, | |
| onChanged: (value) { | |
| setState(() { | |
| selectedValue = value; | |
| }); | |
| }, | |
| child: const Column( | |
| children: [ | |
| NakedRadioButton<String>( | |
| value: 'option1', | |
| child: SizedBox(width: 20, height: 20), | |
| ), | |
| NakedRadioButton<String>( | |
| value: 'option2', | |
| enabled: true, | |
| child: SizedBox(width: 20, height: 20), | |
| ), | |
| ], | |
| ), | |
| ); | |
| }, | |
| ), | |
| ), | |
| ), | |
| ); | |
| // Try to tap the disabled option | |
| await tester.tap(find.byType(NakedRadioButton<String>).at(1)); | |
| await tester.pump(); | |
| // Verify selection did not change | |
| expect(selectedValue, 'option1'); | |
| }); | |
| testWidgets('does not respond when individually disabled', | |
| (WidgetTester tester) async { | |
| String? selectedValue = 'option1'; | |
| // Create a group with buttons where one button is disabled | |
| await tester.pumpWidget( | |
| MaterialApp( | |
| home: Scaffold( | |
| body: StatefulBuilder( | |
| builder: (context, setState) { | |
| return NakedRadioGroup<String>( | |
| value: selectedValue, | |
| onChanged: (value) { | |
| setState(() { | |
| selectedValue = value; | |
| }); | |
| }, | |
| child: const Column( | |
| children: [ | |
| NakedRadioButton<String>( | |
| value: 'option1', | |
| child: SizedBox(width: 20, height: 20), | |
| ), | |
| NakedRadioButton<String>( | |
| value: 'option2', | |
| enabled: false, | |
| child: SizedBox(width: 20, height: 20), | |
| ), | |
| ], | |
| ), | |
| ); | |
| }, | |
| ), | |
| ), | |
| ), | |
| ); | |
| // Try to tap the disabled option | |
| await tester.tap(find.byType(NakedRadioButton<String>).at(1)); | |
| await tester.pump(); | |
| // Verify selection did not change | |
| expect(selectedValue, 'option1'); | |
| }); |
| testWidgets('propagates disabled state to buttons', | ||
| (WidgetTester tester) async { | ||
| String? selectedValue = 'option1'; | ||
| bool callbackCalled = false; | ||
|
|
||
| // Create a group with buttons where the group is disabled | ||
| await tester.pumpWidget( | ||
| MaterialApp( | ||
| home: Scaffold( | ||
| body: StatefulBuilder( | ||
| builder: (context, setState) { | ||
| return NakedRadioGroup<String>( | ||
| value: selectedValue, | ||
| onChanged: (value) { | ||
| setState(() { | ||
| selectedValue = value; | ||
| callbackCalled = true; | ||
| }); | ||
| }, | ||
| enabled: true, | ||
| child: const Column( | ||
| children: [ | ||
| NakedRadioButton<String>( | ||
| value: 'option1', | ||
| child: SizedBox(width: 20, height: 20), | ||
| ), | ||
| NakedRadioButton<String>( | ||
| value: 'option2', | ||
| child: SizedBox(width: 20, height: 20), | ||
| ), | ||
| ], | ||
| ), | ||
| ); | ||
| }, | ||
| ), | ||
| ), | ||
| ), | ||
| ); | ||
|
|
||
| // Try to tap the second option | ||
| await tester.tap(find.byType(NakedRadioButton<String>).at(1)); | ||
| await tester.pump(); | ||
|
|
||
| // Verify callback was not called and value didn't change | ||
| expect(callbackCalled, false); | ||
| expect(selectedValue, 'option1'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test name vs. enabled state mismatch.
The test is titled "propagates disabled state to buttons," but the group is set to enabled: true on line 45, so the assertions effectively test a non-disabled scenario. Either the name should be updated to reflect that it's an "enabled state" test or the group should be enabled: false to align with the test's intention.
- enabled: true,
+ enabled: false,📝 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.
| testWidgets('propagates disabled state to buttons', | |
| (WidgetTester tester) async { | |
| String? selectedValue = 'option1'; | |
| bool callbackCalled = false; | |
| // Create a group with buttons where the group is disabled | |
| await tester.pumpWidget( | |
| MaterialApp( | |
| home: Scaffold( | |
| body: StatefulBuilder( | |
| builder: (context, setState) { | |
| return NakedRadioGroup<String>( | |
| value: selectedValue, | |
| onChanged: (value) { | |
| setState(() { | |
| selectedValue = value; | |
| callbackCalled = true; | |
| }); | |
| }, | |
| enabled: true, | |
| child: const Column( | |
| children: [ | |
| NakedRadioButton<String>( | |
| value: 'option1', | |
| child: SizedBox(width: 20, height: 20), | |
| ), | |
| NakedRadioButton<String>( | |
| value: 'option2', | |
| child: SizedBox(width: 20, height: 20), | |
| ), | |
| ], | |
| ), | |
| ); | |
| }, | |
| ), | |
| ), | |
| ), | |
| ); | |
| // Try to tap the second option | |
| await tester.tap(find.byType(NakedRadioButton<String>).at(1)); | |
| await tester.pump(); | |
| // Verify callback was not called and value didn't change | |
| expect(callbackCalled, false); | |
| expect(selectedValue, 'option1'); | |
| }); | |
| testWidgets('propagates disabled state to buttons', | |
| (WidgetTester tester) async { | |
| String? selectedValue = 'option1'; | |
| bool callbackCalled = false; | |
| // Create a group with buttons where the group is disabled | |
| await tester.pumpWidget( | |
| MaterialApp( | |
| home: Scaffold( | |
| body: StatefulBuilder( | |
| builder: (context, setState) { | |
| return NakedRadioGroup<String>( | |
| value: selectedValue, | |
| onChanged: (value) { | |
| setState(() { | |
| selectedValue = value; | |
| callbackCalled = true; | |
| }); | |
| }, | |
| enabled: false, | |
| child: const Column( | |
| children: [ | |
| NakedRadioButton<String>( | |
| value: 'option1', | |
| child: SizedBox(width: 20, height: 20), | |
| ), | |
| NakedRadioButton<String>( | |
| value: 'option2', | |
| child: SizedBox(width: 20, height: 20), | |
| ), | |
| ], | |
| ), | |
| ); | |
| }, | |
| ), | |
| ), | |
| ), | |
| ); | |
| // Try to tap the second option | |
| await tester.tap(find.byType(NakedRadioButton<String>).at(1)); | |
| await tester.pump(); | |
| // Verify callback was not called and value didn't change | |
| expect(callbackCalled, false); | |
| expect(selectedValue, 'option1'); | |
| }); |
| Widget _buildRadioOption(String value, String label, bool isOptionDisabled) { | ||
| return Row( | ||
| children: [ | ||
| NakedRadioButton<String>( | ||
| value: value, | ||
| enabled: isOptionDisabled, | ||
| child: Container( | ||
| width: 20, | ||
| height: 20, | ||
| decoration: BoxDecoration( | ||
| shape: BoxShape.circle, | ||
| border: Border.all( | ||
| color: _getBorderColor(value, isOptionDisabled), | ||
| width: 2, | ||
| ), | ||
| color: isOptionDisabled ? Colors.grey.shade100 : Colors.white, | ||
| ), | ||
| child: Center( | ||
| child: _selectedOption == value | ||
| ? Container( | ||
| width: 10, | ||
| height: 10, | ||
| decoration: BoxDecoration( | ||
| shape: BoxShape.circle, | ||
| color: isOptionDisabled | ||
| ? Colors.grey.shade300 | ||
| : Colors.blue, | ||
| ), | ||
| ) | ||
| : null, | ||
| ), | ||
| ), | ||
| ), | ||
| const SizedBox(width: 8), | ||
| Text( | ||
| label, | ||
| style: TextStyle( | ||
| color: isOptionDisabled ? Colors.grey.shade400 : Colors.black, | ||
| ), | ||
| ), | ||
| ], | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify enabled/disabled logic in _buildRadioOption.
The enabled property is set to isOptionDisabled, which is counterintuitive as typically "enabled" should be the inverse of "disabled".
Rename the parameter or invert the logic for clarity:
Widget _buildRadioOption(String value, String label, bool isOptionDisabled) {
return Row(
children: [
NakedRadioButton<String>(
value: value,
- enabled: isOptionDisabled,
+ enabled: !isOptionDisabled,Alternatively, rename the parameter for clarity:
-Widget _buildRadioOption(String value, String label, bool isOptionDisabled) {
+Widget _buildRadioOption(String value, String label, bool isDisabled) {
return Row(
children: [
NakedRadioButton<String>(
value: value,
- enabled: isOptionDisabled,
+ enabled: !isDisabled,📝 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.
| Widget _buildRadioOption(String value, String label, bool isOptionDisabled) { | |
| return Row( | |
| children: [ | |
| NakedRadioButton<String>( | |
| value: value, | |
| enabled: isOptionDisabled, | |
| child: Container( | |
| width: 20, | |
| height: 20, | |
| decoration: BoxDecoration( | |
| shape: BoxShape.circle, | |
| border: Border.all( | |
| color: _getBorderColor(value, isOptionDisabled), | |
| width: 2, | |
| ), | |
| color: isOptionDisabled ? Colors.grey.shade100 : Colors.white, | |
| ), | |
| child: Center( | |
| child: _selectedOption == value | |
| ? Container( | |
| width: 10, | |
| height: 10, | |
| decoration: BoxDecoration( | |
| shape: BoxShape.circle, | |
| color: isOptionDisabled | |
| ? Colors.grey.shade300 | |
| : Colors.blue, | |
| ), | |
| ) | |
| : null, | |
| ), | |
| ), | |
| ), | |
| const SizedBox(width: 8), | |
| Text( | |
| label, | |
| style: TextStyle( | |
| color: isOptionDisabled ? Colors.grey.shade400 : Colors.black, | |
| ), | |
| ), | |
| ], | |
| ); | |
| } | |
| Widget _buildRadioOption(String value, String label, bool isOptionDisabled) { | |
| return Row( | |
| children: [ | |
| NakedRadioButton<String>( | |
| value: value, | |
| enabled: !isOptionDisabled, | |
| child: Container( | |
| width: 20, | |
| height: 20, | |
| decoration: BoxDecoration( | |
| shape: BoxShape.circle, | |
| border: Border.all( | |
| color: _getBorderColor(value, isOptionDisabled), | |
| width: 2, | |
| ), | |
| color: isOptionDisabled ? Colors.grey.shade100 : Colors.white, | |
| ), | |
| child: Center( | |
| child: _selectedOption == value | |
| ? Container( | |
| width: 10, | |
| height: 10, | |
| decoration: BoxDecoration( | |
| shape: BoxShape.circle, | |
| color: isOptionDisabled | |
| ? Colors.grey.shade300 | |
| : Colors.blue, | |
| ), | |
| ) | |
| : null, | |
| ), | |
| ), | |
| ), | |
| const SizedBox(width: 8), | |
| Text( | |
| label, | |
| style: TextStyle( | |
| color: isOptionDisabled ? Colors.grey.shade400 : Colors.black, | |
| ), | |
| ), | |
| ], | |
| ); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/naked/lib/src/naked_slider.dart (1)
344-402: Consider unifying keyboard navigation behavior in vertical mode.For horizontal sliders, left/right and up/down keys have distinct behaviors, but for vertical sliders both sets of keys do the same thing. This might be confusing for users expecting consistent control mapping.
Consider this improved approach for vertical slider keyboard handling:
} else { // Vertical slider has inverted up/down controls if (event.logicalKey == LogicalKeyboardKey.arrowUp) { newValue += step; } else if (event.logicalKey == LogicalKeyboardKey.arrowDown) { newValue -= step; - } else if (event.logicalKey == LogicalKeyboardKey.arrowRight) { - newValue += step; - } else if (event.logicalKey == LogicalKeyboardKey.arrowLeft) { - newValue -= step; } else if (event.logicalKey == LogicalKeyboardKey.home) { newValue = widget.min; } else if (event.logicalKey == LogicalKeyboardKey.end) { newValue = widget.max; }packages/naked/example/lib/examples/naked_slider_example.dart (3)
474-474: Fix withValues method in commented code.There's a syntax error in the commented-out range slider code. The
withValuesmethod doesn't exist on Color. UsewithOpacityinstead.- color: Colors.black.withValues(alpha: 0.1), + color: Colors.black.withOpacity(0.1),
1034-1041: Simplify stepped slider value handling.The current implementation has an unnecessary roundToDouble call since the value is already being calculated as a step.
onChanged: (value) { - final stepValue = (value * (_steps.length - 1)).roundToDouble(); + final stepValue = (value * (_steps.length - 1)); setState(() { _value = stepValue; }); },
1321-1321: Add missing import for FontFeature.The code references
FontFeature.tabularFigures()but there's no import for the dart:ui package which contains this class.Add this import at the top of the file:
import 'dart:ui';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/naked/example/lib/examples/naked_slider_example.dart(1 hunks)packages/naked/lib/src/naked_slider.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test Min SDK
- GitHub Check: Test
- GitHub Check: production
🔇 Additional comments (10)
packages/naked/lib/src/naked_slider.dart (7)
121-138: Well-defined property documentation.The clear, detailed documentation for each property properly explains their purpose, making the API easy to understand for developers. Good use of standard naming conventions with meaningful parameter names and default values.
218-252: Good use of InheritedWidget pattern for state management.The
NakedSliderStateclass properly exposes interaction states to descendants through the InheritedWidget pattern, which allows consumers to build reactive UIs. Theofmethod with assertion helps developers catch context-related issues early.
280-288: Well-implemented divisions handling.The division logic correctly normalizes values into discrete steps when the
divisionsparameter is provided. This is essential for sliders that need to snap to specific values.
303-329: Clean implementation of drag update calculations.The drag handling properly calculates value changes based on direction, accounting for both horizontal and vertical sliders. The use of
dragExtentand proper inversion for vertical sliders ensures consistent behavior.
351-354: Good step adjustment for divisions.The code correctly adjusts the step size when divisions are specified, ensuring keyboard navigation works properly with discrete slider steps.
418-492: Excellent accessibility implementation.The slider provides comprehensive accessibility support with semantics information, appropriate keyboard navigation, and interaction states. The use of percentage values for increased/decreased semantics makes the component work well with screen readers.
443-456: Well-handled hover state management.The mouse region implementation properly tracks hover state and conditionally applies cursor styles based on the interactive state. The callback pattern aligns well with the component's design philosophy.
packages/naked/example/lib/examples/naked_slider_example.dart (3)
125-227: Excellent BasicSlider example implementation.The example clearly demonstrates how to access and use the slider state to create a responsive UI. The visual feedback for hover, focus, and dragging states provides a good reference for consumers.
273-345: Good use of LayoutBuilder for responsive design.The SliderWithValue example correctly uses LayoutBuilder to calculate positions based on available space rather than hardcoded values, making it more responsive to different screen sizes.
1224-1304: Well-implemented vertical slider example.This example clearly demonstrates how to implement a vertical slider with the correct positioning calculations and visual feedback. The use of gradient colors in the track adds visual interest.
We have spent a lot of time playing with the idea of having WidgetState; however, this created some challenges that meant we would have to create an opinionated controller to wrap the state of the components. We wanted to try to avoid having any controllers, state management, or opinions outside of its inner state. However, I agree that a controller and/or widget state implementation makes sense, and that would be done outside of naked and using it. TL;DR: Makes sense, but we want to stay away from any controls and states outside of each individual widget. |
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
packages/naked/example/lib/examples/naked_button_example.dart (1)
886-917: Duplicate helper overrides – extract a shared utility
_buildCardContainer(and its stylistic twin_buildSectionTitle) appears in three separate components. Consolidate them into a single widget or utility to honour DRY and ease future tweaks.packages/naked/example/lib/examples/naked_checkbox_example.dart (1)
316-424: Code duplication in checkbox visual builders.The
_buildDefaultCheckboxVisualmethod is duplicated across different widget classes in this file. Consider extracting this into a shared utility class or widget to maintain DRY (Don't Repeat Yourself) principles.Create a utility class or file to share this common functionality:
// In a file like checkbox_visual_utilities.dart class CheckboxVisualBuilder { static Widget buildDefaultCheckboxVisual({ required bool isChecked, bool isIndeterminate = false, bool isHovered = false, bool isPressed = false, bool isFocused = false, bool isDisabled = false, required Color color, }) { // Implementation moved from _buildDefaultCheckboxVisual } } // Then in your widget files: import 'checkbox_visual_utilities.dart'; // Replace the duplicated methods with: CheckboxVisualBuilder.buildDefaultCheckboxVisual( isChecked: _isChecked, isIndeterminate: _isIndeterminate, isHovered: _isHovered, isPressed: _isPressed, isFocused: _isFocused, color: Colors.blue, )
🧹 Nitpick comments (9)
packages/naked/lib/naked.dart (2)
11-14: Replace hard‑coded relative doc link with a public, package‑relative path
.context/plan/naked_component_development_guide.mdis an internal path that won’t resolve for consumers (pub.dev, IDEs, dartdoc).
Expose the guide viapackage:naked/doc/…or an external URL so generated API docs don’t contain a broken link.-/// see the Naked Component Development Guide at: -/// .context/plan/naked_component_development_guide.md +/// See the full development guide at: +/// https://github.com/<org>/naked/blob/main/docs/naked_component_development_guide.md
33-37: Remove duplicate “NakedDialog” from the TODO list
NakedDialogis already a shipped component (exported above). Keeping it in Future components may confuse contributors.-// - NakedDialogpackages/naked/example/lib/examples/textfield_example_page.dart (1)
102-105: UsedebugPrintinstead of
UsingdebugPrintthrottles output and is stripped in release mode.- onPressed: () => print('onTap'), - onPressOutside: (_) => print('onTapOutside'), - onPressUpOutside: (_) => print('onTapUpOutside'), + onPressed: () => debugPrint('onTap'), + onPressOutside: (_) => debugPrint('onTapOutside'), + onPressUpOutside: (_) => debugPrint('onTapUpOutside'),packages/naked/example/lib/main.dart (3)
1-2: Remove leftover commented‐out importLine 1 is a stale, commented duplicate of the active import on line 2. Leaving commented code around clutters diffs and confuses future readers.
-// import 'package:example/examples/naked_tooltip_example.dart';
88-105: Route parsing can break on query‑strings, fragments, or future nested paths
settings.nameis treated as a raw string and manipulated with simplesubstringoperations.
If the URL ever contains a query (/menu?foo=bar) or hash fragment,_getDestinationIndexwill receive an unexpected value and default to index 0, breaking deep‑linking.Consider using
Uri.parse(settings.name!).pathSegmentswhich is battle‑tested for these edge‑cases and removes the manual “substring(1)”:- final path = settings.name ?? '/'; - … - final componentName = path.substring(1); + final uri = Uri.parse(settings.name ?? '/'); + final componentName = + (uri.pathSegments.isEmpty ? '' : uri.pathSegments.first);
108-122:destinations()is recomputed on every call
destinations()allocates a new list each time it is invoked (4× in this file).
Because the list is immutable, you can promote it to aconsttop‑level (or astatic constinsideMyApp) and pay the construction cost only once:-List<NavDestination> destinations() => [ +const List<NavDestination> kDestinations = [ … // unchanged ];Then reference
kDestinationseverywhere.
This also allows the compiler to canonicalise the literals and shrink the binary a bit.packages/naked/example/lib/examples/naked_button_example.dart (1)
963-983: TightsetStateloop may jank – considerTimer.periodic
handleProgressClick()performs tensetStatecalls in quick succession inside an async loop.
On slower devices this can cause missed frames. ATimer.periodicwith a fixed interval (orTicker) avoids awaiting inside the loop and lets the event queue breathe.packages/naked/example/lib/examples/naked_tabs_example.dart (2)
164-164: Remove debug print statements.There are several
- print(' id: $id, focus: $focus'); - print('object'); - print('Panel 1 isFocused: $isFocused'); - print('Panel 2 isFocused: $isFocused'); - print('Panel 3 isFocused: $isFocused'); - print('${widget.tabId} isFocused: $isFocused'); - print('${widget.tabId} isHovered: $isHovered'); - print('Color 1'); - print('Color 2'); - print('Color 3');Also applies to: 222-223, 1328-1329, 1341-1342, 1353-1354, 1390-1391, 1394-1395, 1332-1333, 1344-1345, 1356-1357
179-180: Inconsistent opacity method usage.There's an inconsistency in how opacity is applied to colors. Line 179 uses
.withValues(alpha: 0.5)while line 339 and others use.withOpacity(0.5)for the same purpose. Choose one approach for consistency.- .withValues(alpha: 0.5) + .withOpacity(0.5)Also applies to: 339-340
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
packages/mix/example/.gitignore(1 hunks)packages/naked/example/lib/examples/naked_accordion_example.dart(1 hunks)packages/naked/example/lib/examples/naked_button_example.dart(1 hunks)packages/naked/example/lib/examples/naked_checkbox_example.dart(1 hunks)packages/naked/example/lib/examples/naked_dialog_example.dart(1 hunks)packages/naked/example/lib/examples/naked_menu_example.dart(1 hunks)packages/naked/example/lib/examples/naked_radio_example.dart(1 hunks)packages/naked/example/lib/examples/naked_tabs_example.dart(1 hunks)packages/naked/example/lib/examples/textfield_example_page.dart(1 hunks)packages/naked/example/lib/main.dart(1 hunks)packages/naked/lib/naked.dart(1 hunks)packages/naked/lib/src/naked_dialog.dart(1 hunks)packages/naked/lib/src/naked_menu.dart(1 hunks)packages/naked/lib/src/naked_tabs.dart(1 hunks)packages/naked/lib/src/naked_textfield.dart(1 hunks)packages/naked/lib/src/naked_tooltip.dart(1 hunks)packages/naked/lib/src/utilities/naked_positioning.dart(1 hunks)packages/naked/lib/src/utilities/utilities.dart(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/mix/example/.gitignore
- packages/naked/lib/src/naked_dialog.dart
- packages/naked/lib/src/utilities/naked_positioning.dart
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/naked/lib/src/utilities/utilities.dart
- packages/naked/lib/src/naked_menu.dart
- packages/naked/lib/src/naked_tooltip.dart
- packages/naked/example/lib/examples/naked_menu_example.dart
- packages/naked/example/lib/examples/naked_radio_example.dart
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test Min SDK
- GitHub Check: Test
- GitHub Check: production
🔇 Additional comments (12)
packages/naked/example/lib/examples/naked_dialog_example.dart (1)
256-260: Guardcontext.mountedbehind minimum Flutter SDK version
BuildContext.mountedis only available on Flutter 3.7+.
If the package supports earlier SDKs, this code won’t compile. Either:
- Raise the minimum SDK constraint in
pubspec.yaml, or- Replace with
if (!mounted) return;inside aStatefulWidget.Please verify the intended SDK range.
packages/naked/lib/src/naked_tabs.dart (1)
452-459:excludeSemantics: truehides child semantics from assistive techBecause the surrounding
Semanticsnode setsexcludeSemantics: true, any semantics provided bywidget.child(e.g., an icon with a label) is dropped. If that’s intentional, add a comment; otherwise removeexcludeSemanticsso downstream widgets remain accessible.packages/naked/lib/src/naked_textfield.dart (1)
701-714:rendererIgnoresPointer: truedisables built‑in selection gesturesSetting
rendererIgnoresPointer: truemeans the internalRenderEditablewill never receive pointer events.
Because the outerTextSelectionGestureDetectoris still forwarding taps to_editableText, normal selection handles work, but long‑press selection, drag‑to‑select, and double‑tap word‑selection stop working.Double‑check that this behaviour is intentional for a “headless” field. If not, remove the flag.
packages/naked/example/lib/examples/naked_accordion_example.dart (4)
4-76: Well-organized and comprehensive example structure.The
NakedAccordionExamplewidget is well-structured with a clear layout showcasing multiple accordion variants. The use of a scrollable view with consistent section spacing creates a good user experience for exploring the component.
104-223: Appropriate state management for single accordion expansion.The implementation correctly manages state with
_expandedStates,_hoveredItems, and_focusedItemsmaps. The single expansion mode logic inonExpandedChangeproperly closes other items when expanding a new one, following a good pattern for this interaction type.
577-826: Nested accordions implement proper parent-child state management.The nested accordion implementation correctly handles separate state for parent and child accordions. The use of
_expandedParentItemsand_expandedChildItemsmaintains proper encapsulation between levels while enabling independent expansion states.
1023-1190: Good responsive implementation for card layouts.The card accordion example properly uses
LayoutBuilderto create a responsive layout that adapts between row and column based on screen width. This demonstrates a strong pattern for handling responsiveness with the component.packages/naked/example/lib/examples/naked_checkbox_example.dart (3)
447-593: Good implementation of parent-child checkbox relationship.The
ParentChildCheckboxescomponent demonstrates a well-implemented pattern for managing hierarchical checkbox states. The logic for calculating parent indeterminate states based on child selections is clear and maintainable.
1043-1363: Comprehensive selection example with proper state management.The
SelectionExamplecomponent effectively demonstrates a real-world use case with shopping categories and items. The state management is well-structured and the UI adapts appropriately to the selection state of each item.
1365-1624: Excellent focus management implementation for keyboard accessibility.The
FocusCheckboxExamplecomponent thoroughly demonstrates keyboard navigation and focus management for checkboxes. The visual indicators for focus state and the status display provide good accessibility for keyboard users.packages/naked/example/lib/examples/naked_tabs_example.dart (2)
543-696: Excellent vertical tabs implementation with proper accessibility structure.The vertical tabs example properly implements the orientation parameter and maintains the appropriate
ariarelationships through theNakedTabGroup,NakedTabListandNakedTabcomponents. The visual treatment of active, hover, and focus states provides good feedback to users.
1021-1283: Well-implemented responsive tabs with product filtering.The
ResponsiveTabsGridcomponent demonstrates an excellent pattern for category filtering with tabs. The use ofWrapfor the tab list ensures it works well on various screen sizes, and the product grid adapts appropriately to the selected category.
| /// Focus management is handled through NakedFocusManager which provides: | ||
| /// - Keyboard navigation between tabs using arrow keys | ||
| /// - Home/End keys for navigating to first/last tab | ||
| /// - Proper focus handling for accessibility | ||
| /// |
There was a problem hiding this comment.
Docstring promises Home/End key support that is not implemented
Lines 11‑13 list Home/End navigation, but _handleKeyEvent never handles those keys.
Either implement the logic or update the docs to avoid misleading consumers.
Example patch:
@@
- /// - Home/End keys for navigating to first/last tab
+ /// - Home/End keys for navigating to first/last tab *(coming soon)*—or— implement key handling in _handleKeyEvent.
| if (event is KeyDownEvent) { | ||
| switch (tabsScope.orientation) { | ||
| case Axis.horizontal: | ||
| if (event.logicalKey == LogicalKeyboardKey.arrowLeft) { | ||
| FocusTraversalGroup.of(context).previous(_focusNode); | ||
| return KeyEventResult.handled; | ||
| } | ||
| if (event.logicalKey == LogicalKeyboardKey.arrowRight) { | ||
| FocusTraversalGroup.of(context).next(_focusNode); | ||
| return KeyEventResult.handled; | ||
| } | ||
| break; | ||
| case Axis.vertical: | ||
| if (event.logicalKey == LogicalKeyboardKey.arrowDown) { | ||
| FocusTraversalGroup.of(context).next(_focusNode); | ||
| return KeyEventResult.handled; | ||
| } | ||
| if (event.logicalKey == LogicalKeyboardKey.arrowUp) { | ||
| FocusTraversalGroup.of(context).previous(_focusNode); | ||
| return KeyEventResult.handled; | ||
| } | ||
| break; | ||
| } | ||
| if (event.logicalKey.isConfirmationKey) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add Home/End key handling for first/last tab navigation
Implementing this fulfills the API contract and improves accessibility parity with native tab widgets.
+ if (event.logicalKey == LogicalKeyboardKey.home) {
+ FocusTraversalGroup.of(context).previous(_focusNode, wrap: false);
+ return KeyEventResult.handled;
+ }
+ if (event.logicalKey == LogicalKeyboardKey.end) {
+ FocusTraversalGroup.of(context).next(_focusNode, wrap: false);
+ return KeyEventResult.handled;
+ }📝 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 (event is KeyDownEvent) { | |
| switch (tabsScope.orientation) { | |
| case Axis.horizontal: | |
| if (event.logicalKey == LogicalKeyboardKey.arrowLeft) { | |
| FocusTraversalGroup.of(context).previous(_focusNode); | |
| return KeyEventResult.handled; | |
| } | |
| if (event.logicalKey == LogicalKeyboardKey.arrowRight) { | |
| FocusTraversalGroup.of(context).next(_focusNode); | |
| return KeyEventResult.handled; | |
| } | |
| break; | |
| case Axis.vertical: | |
| if (event.logicalKey == LogicalKeyboardKey.arrowDown) { | |
| FocusTraversalGroup.of(context).next(_focusNode); | |
| return KeyEventResult.handled; | |
| } | |
| if (event.logicalKey == LogicalKeyboardKey.arrowUp) { | |
| FocusTraversalGroup.of(context).previous(_focusNode); | |
| return KeyEventResult.handled; | |
| } | |
| break; | |
| } | |
| if (event.logicalKey.isConfirmationKey) { | |
| if (event is KeyDownEvent) { | |
| switch (tabsScope.orientation) { | |
| case Axis.horizontal: | |
| if (event.logicalKey == LogicalKeyboardKey.arrowLeft) { | |
| FocusTraversalGroup.of(context).previous(_focusNode); | |
| return KeyEventResult.handled; | |
| } | |
| if (event.logicalKey == LogicalKeyboardKey.arrowRight) { | |
| FocusTraversalGroup.of(context).next(_focusNode); | |
| return KeyEventResult.handled; | |
| } | |
| break; | |
| case Axis.vertical: | |
| if (event.logicalKey == LogicalKeyboardKey.arrowDown) { | |
| FocusTraversalGroup.of(context).next(_focusNode); | |
| return KeyEventResult.handled; | |
| } | |
| if (event.logicalKey == LogicalKeyboardKey.arrowUp) { | |
| FocusTraversalGroup.of(context).previous(_focusNode); | |
| return KeyEventResult.handled; | |
| } | |
| break; | |
| } | |
| + // Jump to first/last tab on Home/End | |
| + if (event.logicalKey == LogicalKeyboardKey.home) { | |
| + FocusTraversalGroup.of(context).previous(_focusNode, wrap: false); | |
| + return KeyEventResult.handled; | |
| + } | |
| + if (event.logicalKey == LogicalKeyboardKey.end) { | |
| + FocusTraversalGroup.of(context).next(_focusNode, wrap: false); | |
| + return KeyEventResult.handled; | |
| + } | |
| if (event.logicalKey.isConfirmationKey) { | |
| // … | |
| } | |
| } |
| @override | ||
| bool get onUserTapAlwaysCalled => _state.widget.onPressedAlwaysCalled; | ||
|
|
||
| @override | ||
| void onUserTap() { | ||
| _state.widget.onPressed?.call(); | ||
| } | ||
|
|
||
| @override | ||
| void onTapDown(TapDragDownDetails details) { | ||
| super.onTapDown(details); | ||
| _state.widget.onPressedState?.call(true); | ||
| } | ||
|
|
||
| @override | ||
| void onSingleTapUp(TapDragUpDetails details) { | ||
| super.onSingleTapUp(details); | ||
| _state.widget.onPressedState?.call(false); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pressed‑state callback isn’t reset on gesture cancel
onTapDown sets onPressedState(true) and onSingleTapUp resets it to false, but a drag‑cancel (e.g. the user moves the finger off the button) leaves the state stuck at true.
Hook onTapCancel as well:
@override
void onTapDown(TapDragDownDetails details) {
super.onTapDown(details);
_state.widget.onPressedState?.call(true);
}
+ @override
+ void onTapCancel() {
+ super.onTapCancel();
+ _state.widget.onPressedState?.call(false);
+ }📝 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.
| @override | |
| bool get onUserTapAlwaysCalled => _state.widget.onPressedAlwaysCalled; | |
| @override | |
| void onUserTap() { | |
| _state.widget.onPressed?.call(); | |
| } | |
| @override | |
| void onTapDown(TapDragDownDetails details) { | |
| super.onTapDown(details); | |
| _state.widget.onPressedState?.call(true); | |
| } | |
| @override | |
| void onSingleTapUp(TapDragUpDetails details) { | |
| super.onSingleTapUp(details); | |
| _state.widget.onPressedState?.call(false); | |
| } | |
| @override | |
| bool get onUserTapAlwaysCalled => _state.widget.onPressedAlwaysCalled; | |
| @override | |
| void onUserTap() { | |
| _state.widget.onPressed?.call(); | |
| } | |
| @override | |
| void onTapDown(TapDragDownDetails details) { | |
| super.onTapDown(details); | |
| _state.widget.onPressedState?.call(true); | |
| } | |
| @override | |
| void onTapCancel() { | |
| super.onTapCancel(); | |
| _state.widget.onPressedState?.call(false); | |
| } | |
| @override | |
| void onSingleTapUp(TapDragUpDetails details) { | |
| super.onSingleTapUp(details); | |
| _state.widget.onPressedState?.call(false); | |
| } |
| enableInteractiveSelection: widget.enableInteractiveSelection, | ||
| dragStartBehavior: widget.dragStartBehavior, | ||
| scrollController: widget.scrollController, | ||
| scrollPhysics: widget.scrollPhysics, | ||
| autofillClient: this, | ||
| clipBehavior: widget.clipBehavior, | ||
| restorationId: 'editable', | ||
| stylusHandwritingEnabled: widget.stylusHandwritingEnabled, | ||
| enableIMEPersonalizedLearning: widget.enableIMEPersonalizedLearning, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hard‑coded restoration ID can collide when multiple fields are used
EditableText is given the constant restorationId: 'editable'.
If a page contains two NakedTextFields, Flutter’s state‑restoration subsystem will throw because IDs must be unique per route.
Derive the ID from widget.restorationId (when provided) or fall back to null:
- restorationId: 'editable',
+ restorationId: widget.restorationId == null
+ ? null
+ : '${widget.restorationId}_editable',📝 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.
| enableInteractiveSelection: widget.enableInteractiveSelection, | |
| dragStartBehavior: widget.dragStartBehavior, | |
| scrollController: widget.scrollController, | |
| scrollPhysics: widget.scrollPhysics, | |
| autofillClient: this, | |
| clipBehavior: widget.clipBehavior, | |
| restorationId: 'editable', | |
| stylusHandwritingEnabled: widget.stylusHandwritingEnabled, | |
| enableIMEPersonalizedLearning: widget.enableIMEPersonalizedLearning, | |
| enableInteractiveSelection: widget.enableInteractiveSelection, | |
| dragStartBehavior: widget.dragStartBehavior, | |
| scrollController: widget.scrollController, | |
| scrollPhysics: widget.scrollPhysics, | |
| autofillClient: this, | |
| clipBehavior: widget.clipBehavior, | |
| - restorationId: 'editable', | |
| + restorationId: widget.restorationId == null | |
| + ? null | |
| + : '${widget.restorationId}_editable', | |
| stylusHandwritingEnabled: widget.stylusHandwritingEnabled, | |
| enableIMEPersonalizedLearning: widget.enableIMEPersonalizedLearning, |
| // Configure platform-specific properties | ||
| switch (theme.platform) { | ||
| case TargetPlatform.iOS: | ||
| forcePressEnabled = true; | ||
| textSelectionControls ??= cupertinoTextSelectionHandleControls; | ||
| paintCursorAboveText = true; | ||
| cursorOpacityAnimates ??= true; | ||
| cursorColor = widget.cursorColor ?? CupertinoColors.activeBlue; | ||
| selectionColor = CupertinoColors.activeBlue.withOpacity(0.40); | ||
| cursorRadius ??= const Radius.circular(2.0); | ||
| cursorOffset = Offset( | ||
| iOSHorizontalOffset / MediaQuery.devicePixelRatioOf(context), | ||
| 0, | ||
| ); | ||
| case TargetPlatform.macOS: | ||
| forcePressEnabled = false; | ||
| textSelectionControls ??= cupertinoDesktopTextSelectionHandleControls; | ||
| paintCursorAboveText = true; | ||
| cursorOpacityAnimates ??= false; | ||
| cursorColor = widget.cursorColor ?? CupertinoColors.activeBlue; | ||
| selectionColor = CupertinoColors.activeBlue.withOpacity(0.40); | ||
| cursorRadius ??= const Radius.circular(2.0); | ||
| cursorOffset = Offset( | ||
| iOSHorizontalOffset / MediaQuery.devicePixelRatioOf(context), | ||
| 0, | ||
| ); | ||
| case TargetPlatform.android: | ||
| case TargetPlatform.fuchsia: | ||
| forcePressEnabled = false; | ||
| textSelectionControls ??= materialTextSelectionHandleControls; | ||
| paintCursorAboveText = false; | ||
| cursorOpacityAnimates ??= false; | ||
| cursorColor = widget.cursorColor ?? theme.colorScheme.primary; | ||
| selectionColor = theme.colorScheme.primary.withOpacity(0.40); | ||
| case TargetPlatform.linux: | ||
| case TargetPlatform.windows: | ||
| forcePressEnabled = false; | ||
| textSelectionControls ??= desktopTextSelectionHandleControls; | ||
| paintCursorAboveText = false; | ||
| cursorOpacityAnimates ??= false; | ||
| cursorColor = widget.cursorColor ?? theme.colorScheme.primary; | ||
| selectionColor = theme.colorScheme.primary.withOpacity(0.40); | ||
| } |
There was a problem hiding this comment.
Switch cases are missing break / return – this will not compile
Dart forbids implicit fall‑through between non‑empty case clauses.
Every branch here executes its statements and then falls into the next branch, triggering a compile‑time error (Switch case may fall through). Add an explicit break; (or return) after each case or merge the platforms with pattern matching.
- case TargetPlatform.iOS:
+ case TargetPlatform.iOS:
forcePressEnabled = true;
…
cursorOffset = Offset(
iOSHorizontalOffset / MediaQuery.devicePixelRatioOf(context),
0,
);
+ break;Apply the same fix for every subsequent case in this switch.
📝 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.
| // Configure platform-specific properties | |
| switch (theme.platform) { | |
| case TargetPlatform.iOS: | |
| forcePressEnabled = true; | |
| textSelectionControls ??= cupertinoTextSelectionHandleControls; | |
| paintCursorAboveText = true; | |
| cursorOpacityAnimates ??= true; | |
| cursorColor = widget.cursorColor ?? CupertinoColors.activeBlue; | |
| selectionColor = CupertinoColors.activeBlue.withOpacity(0.40); | |
| cursorRadius ??= const Radius.circular(2.0); | |
| cursorOffset = Offset( | |
| iOSHorizontalOffset / MediaQuery.devicePixelRatioOf(context), | |
| 0, | |
| ); | |
| case TargetPlatform.macOS: | |
| forcePressEnabled = false; | |
| textSelectionControls ??= cupertinoDesktopTextSelectionHandleControls; | |
| paintCursorAboveText = true; | |
| cursorOpacityAnimates ??= false; | |
| cursorColor = widget.cursorColor ?? CupertinoColors.activeBlue; | |
| selectionColor = CupertinoColors.activeBlue.withOpacity(0.40); | |
| cursorRadius ??= const Radius.circular(2.0); | |
| cursorOffset = Offset( | |
| iOSHorizontalOffset / MediaQuery.devicePixelRatioOf(context), | |
| 0, | |
| ); | |
| case TargetPlatform.android: | |
| case TargetPlatform.fuchsia: | |
| forcePressEnabled = false; | |
| textSelectionControls ??= materialTextSelectionHandleControls; | |
| paintCursorAboveText = false; | |
| cursorOpacityAnimates ??= false; | |
| cursorColor = widget.cursorColor ?? theme.colorScheme.primary; | |
| selectionColor = theme.colorScheme.primary.withOpacity(0.40); | |
| case TargetPlatform.linux: | |
| case TargetPlatform.windows: | |
| forcePressEnabled = false; | |
| textSelectionControls ??= desktopTextSelectionHandleControls; | |
| paintCursorAboveText = false; | |
| cursorOpacityAnimates ??= false; | |
| cursorColor = widget.cursorColor ?? theme.colorScheme.primary; | |
| selectionColor = theme.colorScheme.primary.withOpacity(0.40); | |
| } | |
| // Configure platform-specific properties | |
| switch (theme.platform) { | |
| case TargetPlatform.iOS: | |
| forcePressEnabled = true; | |
| textSelectionControls ??= cupertinoTextSelectionHandleControls; | |
| paintCursorAboveText = true; | |
| cursorOpacityAnimates ??= true; | |
| cursorColor = widget.cursorColor ?? CupertinoColors.activeBlue; | |
| selectionColor = CupertinoColors.activeBlue.withOpacity(0.40); | |
| cursorRadius ??= const Radius.circular(2.0); | |
| cursorOffset = Offset( | |
| iOSHorizontalOffset / MediaQuery.devicePixelRatioOf(context), | |
| 0, | |
| ); | |
| break; | |
| case TargetPlatform.macOS: | |
| forcePressEnabled = false; | |
| textSelectionControls ??= cupertinoDesktopTextSelectionHandleControls; | |
| paintCursorAboveText = true; | |
| cursorOpacityAnimates ??= false; | |
| cursorColor = widget.cursorColor ?? CupertinoColors.activeBlue; | |
| selectionColor = CupertinoColors.activeBlue.withOpacity(0.40); | |
| cursorRadius ??= const Radius.circular(2.0); | |
| cursorOffset = Offset( | |
| iOSHorizontalOffset / MediaQuery.devicePixelRatioOf(context), | |
| 0, | |
| ); | |
| break; | |
| case TargetPlatform.android: | |
| case TargetPlatform.fuchsia: | |
| forcePressEnabled = false; | |
| textSelectionControls ??= materialTextSelectionHandleControls; | |
| paintCursorAboveText = false; | |
| cursorOpacityAnimates ??= false; | |
| cursorColor = widget.cursorColor ?? theme.colorScheme.primary; | |
| selectionColor = theme.colorScheme.primary.withOpacity(0.40); | |
| break; | |
| case TargetPlatform.linux: | |
| case TargetPlatform.windows: | |
| forcePressEnabled = false; | |
| textSelectionControls ??= desktopTextSelectionHandleControls; | |
| paintCursorAboveText = false; | |
| cursorOpacityAnimates ??= false; | |
| cursorColor = widget.cursorColor ?? theme.colorScheme.primary; | |
| selectionColor = theme.colorScheme.primary.withOpacity(0.40); | |
| break; | |
| } |
| Widget _buildGridLayout({ | ||
| required int columnCount, | ||
| required Widget child, | ||
| }) { | ||
| return Container( | ||
| width: double.infinity, | ||
| padding: const EdgeInsets.all(8.0), | ||
| decoration: BoxDecoration( | ||
| color: Colors.grey[50], | ||
| borderRadius: BorderRadius.circular(8), | ||
| ), | ||
| child: child, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
columnCount parameter is never used
_buildGridLayout receives columnCount but ignores it, so callers think they can influence the layout while nothing happens.
Either wire it up (e.g. replace Container with a GridView.count) or drop the parameter to avoid confusion.
-Widget _buildGridLayout({
- required int columnCount,
- required Widget child,
-}) {
+Widget _buildGridLayout({
+ required int columnCount,
+ required Widget child,
+}) {
return Container(Committable suggestion skipped: line range outside the PR's diff.
| tabId: 'tab2', | ||
| label: 'Tab 2', | ||
| ), | ||
| Tab( | ||
| tabId: 'tab3', | ||
| label: 'Tab 3', | ||
| ), | ||
| ], | ||
| ), | ||
| ), | ||
| NakedTabPanel( | ||
| tabId: 'tab1', | ||
| child: TextButton( | ||
| onFocusChange: (isFocused) { | ||
| print('Panel 1 isFocused: $isFocused'); | ||
| }, | ||
| child: const Text('Panel 1'), | ||
| onPressed: () { | ||
| print('Color 1'); | ||
| }, | ||
| ), | ||
| ), | ||
| NakedTabPanel( | ||
| tabId: 'tab2', | ||
| child: TextButton( | ||
| child: const Text('Panel 2'), | ||
| onFocusChange: (isFocused) { | ||
| print('Panel 2 isFocused: $isFocused'); | ||
| }, | ||
| onPressed: () { | ||
| print('Color 2'); | ||
| }, | ||
| ), | ||
| ), | ||
| NakedTabPanel( | ||
| tabId: 'tab3', | ||
| child: TextButton( | ||
| child: const Text('Panel 3'), | ||
| onFocusChange: (isFocused) { | ||
| print('Panel 3 isFocused: $isFocused'); | ||
| }, | ||
| onPressed: () { | ||
| print('Color 3'); | ||
| }, | ||
| ), | ||
| ), | ||
| ], | ||
| ), | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| class Tab extends StatefulWidget { | ||
| const Tab({ | ||
| super.key, | ||
| required this.tabId, | ||
| required this.label, | ||
| }); | ||
|
|
||
| final String tabId; | ||
| final String label; | ||
|
|
||
| @override | ||
| State<Tab> createState() => _TabState(); | ||
| } | ||
|
|
||
| class _TabState extends State<Tab> { | ||
| bool isSelected = false; | ||
| bool isSemiSelected = false; | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| return NakedTab( | ||
| onFocusState: (isFocused) => setState(() { | ||
| isSemiSelected = isFocused; | ||
| print('${widget.tabId} isFocused: $isFocused'); | ||
| }), | ||
| onHoverState: (isHovered) => setState(() { | ||
| isSemiSelected = isHovered; | ||
| print('${widget.tabId} isHovered: $isHovered'); | ||
| }), | ||
| tabId: widget.tabId, | ||
| child: Container( | ||
| decoration: BoxDecoration( | ||
| color: isSemiSelected | ||
| ? Colors.red.withAlpha(100) | ||
| : isSelected | ||
| ? Colors.blue.withAlpha(100) | ||
| : Colors.transparent, | ||
| borderRadius: BorderRadius.circular(100), | ||
| ), | ||
| padding: const EdgeInsets.all(10), | ||
| child: Text( | ||
| widget.label, | ||
| ), | ||
| ), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing or documenting the experimental MyApp and Tab classes.
The MyApp and Tab classes at the end of the file appear disconnected from the main examples and may confuse users. Either remove this code if it's not meant to be part of the example, or add documentation explaining its purpose.
If these classes are meant to be part of the example, consider:
- Adding proper documentation explaining their purpose
- Integrating them with the main example structure
- Removing debug print statements
If they're not meant to be included, they should be removed entirely.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
packages/mix/example/lib/main.dart (2)
40-51: Excellent responsive styling implementationThe style function effectively demonstrates several key concepts:
- Responsive layout with breakpoint conditions
- Animation with duration and curve
- Fluent API for style composition
Consider adding a comment explaining the breakpoint logic for clarity, particularly why the 0-365px range was chosen.
+ // Switch to vertical layout on very small screens (under 365px width) $on.breakpoint(const Breakpoint(minWidth: 0, maxWidth: 365))( $flexbox.chain.flex.direction(Axis.vertical), ),
7-16: Show usage of primary color token in stylingYou've defined a primary color token but don't demonstrate its usage in the style function. Consider adding an example of how to reference the theme color.
Style style() => Style( $icon.color.red(), + // Using themed primary color from MixTheme + $text.color(primary), $flexbox.chain ..flex.direction(Axis.horizontal) ..flex.mainAxisSize.min(),Also applies to: 40-51
packages/naked/example/lib/examples/naked_slider_example.dart (2)
43-46: Consider implementing the commented out RangeSlider sectionThe RangeSlider section is currently commented out. If this component is stable and ready for use, consider uncommenting and finalizing it to provide a more comprehensive set of examples.
117-1322: Add explanatory comments for educational purposesSince this is an example file meant to demonstrate the usage of NakedSlider, consider adding more explanatory comments throughout the implementations. This would help developers understand key concepts and implementation details.
For example:
// Thumb Positioned( left: (_value / 100) * constraints.maxWidth - 8, + // Offset by half the thumb width to center it on the value point child: Container( width: isDragging ? 24 : 20, height: isDragging ? 24 : 20, + // Increase size when dragging for better visual feedback decoration: BoxDecoration(packages/naked/example/lib/examples/naked_select_example.dart (1)
206-236: Simplify trigger state handling – avoid 3× setState on identical callbackInside
NakedSelectTriggeryou register three callbacks
onHoverState,onFocusState,onPressedStatethat each do nothing but
setState(() => _isBasicTriggerHovered = value)(etc.).
While functional, this induces three separate widget rebuilds for a single
pointer interaction.Consider batching them into a single local
void _updateTriggerStates({...})
method or using a smallTriggerVisualStatedata class to update all three
flags at once, reducing build passes and improving animation smoothness
(especially noticeable in web builds).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/mix/example/lib/main.dart(1 hunks)packages/naked/example/lib/examples/naked_select_example.dart(1 hunks)packages/naked/example/lib/examples/naked_slider_example.dart(1 hunks)packages/naked/example/lib/main.dart(1 hunks)packages/naked/lib/src/naked_select.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/naked/example/lib/main.dart
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test
- GitHub Check: Test Min SDK
- GitHub Check: production
🔇 Additional comments (5)
packages/mix/example/lib/main.dart (3)
4-5: Good use of color tokenizationUsing a color token for 'primary' follows best practices for maintainable theming. This approach aligns well with the design system philosophy.
7-16: Well-structured theme implementationThe
MixThemewrapper provides a clean way to define theme data and make it available throughout the application. This implementation demonstrates good separation of styling from behavior, which aligns with the PR's "Naked" component philosophy.
24-36: Consider internationalization implicationsUsing
Directionalitydirectly with hardcodedTextDirection.ltrworks for this example but might not be suitable for apps that need to support right-to-left languages.The approach of using
FlexBoxwith a dynamic style function demonstrates good separation of layout from styling logic, which aligns well with the PR objectives.packages/naked/example/lib/examples/naked_slider_example.dart (1)
1164-1320: Good implementation of the vertical sliderThe vertical slider implementation effectively demonstrates the directional flexibility of the NakedSlider. Nice touch using tabular figures (
FontFeature.tabularFigures()) for the numeric display to prevent layout shifts when the value changes.packages/naked/lib/src/naked_select.dart (1)
327-335:⚠️ Potential issueDo not swallow non‑handled printable key events when type‑ahead is disabled
_handleTypeAheadearly‑returns whenenableTypeAheadis false, but the
surrounding logic still returnsKeyEventResult.handled, preventing ancestor
widgets from reacting to the keystroke.- if (character != null && - character.isNotEmpty && - widget.enableTypeAhead) { - _handleTypeAhead(character); - return KeyEventResult.handled; - } + if (character != null && character.isNotEmpty) { + if (widget.enableTypeAhead) { + _handleTypeAhead(character); + return KeyEventResult.handled; + } + // allow others to process the key + return KeyEventResult.ignored; + }This respects the user’s configuration and propagates key events correctly.
Likely an incorrect or invalid review comment.
| // boxShadow: [ | ||
| // if (isHovered || isFocused || isDragging) | ||
| // BoxShadow( | ||
| // color: Colors.black.withValues(alpha: 0.1), |
There was a problem hiding this comment.
Fix error in commented code
There's a syntax error in the commented RangeSlider code: withValues(alpha: 0.1) should be withOpacity(0.1).
-color: Colors.black.withValues(alpha: 0.1),
+color: Colors.black.withOpacity(0.1),📝 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.
| // color: Colors.black.withValues(alpha: 0.1), | |
| // color: Colors.black.withOpacity(0.1), |
| @override | ||
| Widget build(BuildContext context) { | ||
| return Column( | ||
| crossAxisAlignment: CrossAxisAlignment.start, | ||
| children: [ | ||
| const Text( | ||
| 'A simple slider with default styling', | ||
| style: TextStyle( | ||
| fontSize: 14, | ||
| color: Color(0xFF4B5563), // text-gray-600 | ||
| ), | ||
| ), | ||
| const SizedBox(height: 16), | ||
| NakedSlider( | ||
| value: _value / 100, | ||
| onChanged: (value) => setState(() { | ||
| _value = value * 100; | ||
| }), | ||
| child: Builder( | ||
| builder: (context) { | ||
| final sliderState = NakedSliderState.of(context); | ||
| final isHovered = sliderState.isHovered; | ||
| final isFocused = sliderState.isFocused; | ||
| final isDragging = sliderState.isDragging; | ||
|
|
||
| return SizedBox( | ||
| width: 300, | ||
| height: 16, | ||
| child: Stack( | ||
| alignment: Alignment.center, | ||
| clipBehavior: Clip.none, | ||
| children: [ | ||
| // Track background | ||
| Container( | ||
| height: 8, | ||
| decoration: BoxDecoration( | ||
| color: const Color(0xFFE5E7EB), // bg-gray-200 | ||
| borderRadius: BorderRadius.circular(4), | ||
| ), | ||
| ), | ||
|
|
||
| // Active track | ||
| Align( | ||
| alignment: Alignment.centerLeft, | ||
| child: FractionallySizedBox( | ||
| widthFactor: _value / 100, | ||
| child: Container( | ||
| height: 8, | ||
| decoration: BoxDecoration( | ||
| color: isFocused | ||
| ? const Color.fromARGB(255, 40, 91, 174) | ||
| : const Color(0xFF3B82F6), | ||
| borderRadius: BorderRadius.circular(4), | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
|
|
||
| // Thumb | ||
| Positioned( | ||
| left: (_value / 100) * 300 - 8, | ||
| child: Container( | ||
| width: isDragging ? 20 : 16, | ||
| height: isDragging ? 20 : 16, | ||
| decoration: BoxDecoration( | ||
| color: const Color(0xFF3B82F6), // bg-blue-500 | ||
| shape: BoxShape.circle, | ||
| border: Border.all( | ||
| color: Colors.white, | ||
| width: 2, | ||
| ), | ||
| boxShadow: [ | ||
| if (isHovered || isFocused || isDragging) | ||
| BoxShadow( | ||
| color: Colors.black.withOpacity(0.1), | ||
| blurRadius: 4, | ||
| offset: const Offset(0, 2), | ||
| ), | ||
| ], | ||
| ), | ||
| ), | ||
| ), | ||
| ], | ||
| ), | ||
| ); | ||
| }, | ||
| ), | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use LayoutBuilder for responsive slider width
The BasicSlider implementation uses a fixed width of 300px for calculations, which could cause positioning issues if the container width changes.
Replace the fixed width with LayoutBuilder like you did in other slider examples:
-return SizedBox(
- width: 300,
- height: 16,
- child: Stack(
+return LayoutBuilder(
+ builder: (context, constraints) {
+ return SizedBox(
+ width: constraints.maxWidth,
+ height: 16,
+ child: Stack(Then update the thumb positioning:
-Positioned(
- left: (_value / 100) * 300 - 8,
+Positioned(
+ left: (_value / 100) * constraints.maxWidth - 8,📝 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.
| @override | |
| Widget build(BuildContext context) { | |
| return Column( | |
| crossAxisAlignment: CrossAxisAlignment.start, | |
| children: [ | |
| const Text( | |
| 'A simple slider with default styling', | |
| style: TextStyle( | |
| fontSize: 14, | |
| color: Color(0xFF4B5563), // text-gray-600 | |
| ), | |
| ), | |
| const SizedBox(height: 16), | |
| NakedSlider( | |
| value: _value / 100, | |
| onChanged: (value) => setState(() { | |
| _value = value * 100; | |
| }), | |
| child: Builder( | |
| builder: (context) { | |
| final sliderState = NakedSliderState.of(context); | |
| final isHovered = sliderState.isHovered; | |
| final isFocused = sliderState.isFocused; | |
| final isDragging = sliderState.isDragging; | |
| return SizedBox( | |
| width: 300, | |
| height: 16, | |
| child: Stack( | |
| alignment: Alignment.center, | |
| clipBehavior: Clip.none, | |
| children: [ | |
| // Track background | |
| Container( | |
| height: 8, | |
| decoration: BoxDecoration( | |
| color: const Color(0xFFE5E7EB), // bg-gray-200 | |
| borderRadius: BorderRadius.circular(4), | |
| ), | |
| ), | |
| // Active track | |
| Align( | |
| alignment: Alignment.centerLeft, | |
| child: FractionallySizedBox( | |
| widthFactor: _value / 100, | |
| child: Container( | |
| height: 8, | |
| decoration: BoxDecoration( | |
| color: isFocused | |
| ? const Color.fromARGB(255, 40, 91, 174) | |
| : const Color(0xFF3B82F6), | |
| borderRadius: BorderRadius.circular(4), | |
| ), | |
| ), | |
| ), | |
| ), | |
| // Thumb | |
| Positioned( | |
| left: (_value / 100) * 300 - 8, | |
| child: Container( | |
| width: isDragging ? 20 : 16, | |
| height: isDragging ? 20 : 16, | |
| decoration: BoxDecoration( | |
| color: const Color(0xFF3B82F6), // bg-blue-500 | |
| shape: BoxShape.circle, | |
| border: Border.all( | |
| color: Colors.white, | |
| width: 2, | |
| ), | |
| boxShadow: [ | |
| if (isHovered || isFocused || isDragging) | |
| BoxShadow( | |
| color: Colors.black.withOpacity(0.1), | |
| blurRadius: 4, | |
| offset: const Offset(0, 2), | |
| ), | |
| ], | |
| ), | |
| ), | |
| ), | |
| ], | |
| ), | |
| ); | |
| }, | |
| ), | |
| ), | |
| child: Builder( | |
| builder: (context) { | |
| final sliderState = NakedSliderState.of(context); | |
| final isHovered = sliderState.isHovered; | |
| final isFocused = sliderState.isFocused; | |
| final isDragging = sliderState.isDragging; | |
| return LayoutBuilder( | |
| builder: (context, constraints) { | |
| return SizedBox( | |
| width: constraints.maxWidth, | |
| height: 16, | |
| child: Stack( | |
| alignment: Alignment.center, | |
| clipBehavior: Clip.none, | |
| children: [ | |
| // Track background | |
| Container( | |
| height: 8, | |
| decoration: BoxDecoration( | |
| color: const Color(0xFFE5E7EB), // bg-gray-200 | |
| borderRadius: BorderRadius.circular(4), | |
| ), | |
| ), | |
| // Active track | |
| Align( | |
| alignment: Alignment.centerLeft, | |
| child: FractionallySizedBox( | |
| widthFactor: _value / 100, | |
| child: Container( | |
| height: 8, | |
| decoration: BoxDecoration( | |
| color: isFocused | |
| ? const Color.fromARGB(255, 40, 91, 174) | |
| : const Color(0xFF3B82F6), | |
| borderRadius: BorderRadius.circular(4), | |
| ), | |
| ), | |
| ), | |
| ), | |
| // Thumb | |
| Positioned( | |
| left: (_value / 100) * constraints.maxWidth - 8, | |
| child: Container( | |
| width: isDragging ? 20 : 16, | |
| height: isDragging ? 20 : 16, | |
| decoration: BoxDecoration( | |
| color: const Color(0xFF3B82F6), // bg-blue-500 | |
| shape: BoxShape.circle, | |
| border: Border.all( | |
| color: Colors.white, | |
| width: 2, | |
| ), | |
| boxShadow: [ | |
| if (isHovered || isFocused || isDragging) | |
| BoxShadow( | |
| color: Colors.black.withOpacity(0.1), | |
| blurRadius: 4, | |
| offset: const Offset(0, 2), | |
| ), | |
| ], | |
| ), | |
| ), | |
| ), | |
| ], | |
| ), | |
| ); | |
| }, | |
| ); | |
| }, | |
| ), |
| final bool _isBasicOpen = false; | ||
| String? _basicSelectedValue; | ||
|
|
||
| // Multiple select state | ||
| final bool _isMultipleOpen = false; | ||
| Set<String> _multipleSelectedValues = {}; | ||
|
|
||
| // Disabled select state | ||
| final bool _isDisabledOpen = false; | ||
| String? _disabledSelectedValue; | ||
|
|
||
| // Search select state | ||
| final bool _isSearchOpen = false; | ||
| String? _searchSelectedValue; | ||
| late final TextEditingController _searchController; | ||
| String _searchText = ''; | ||
| final FocusNode _searchFocusNode = FocusNode(); | ||
| int _keyboardSelectedIndex = -1; | ||
|
|
There was a problem hiding this comment.
_is*Open flags declared as final never change – trigger arrow icon will never update
All four _is*Open booleans are declared final and are never mutated.
They are passed to _buildTrigger() to decide which arrow icon to paint, but, being
constant false, the trigger always shows the closed state even while the menu is
open.
Either:
- Drop these fields altogether and derive the open/closed state from the
NakedSelect’s overlay controller (expose aisOpencallback on the select), or - Make them mutable (
late bool _isBasicOpen = false;) and update them via
onMenuClose:/onMenuOpen:callbacks you add to the select.
-final bool _isBasicOpen = false;
+bool _isBasicOpen = false; // mutable
…
NakedSelect<String>(
+ onMenuOpen: () => setState(() => _isBasicOpen = true),
+ onMenuClose: () => setState(() => _isBasicOpen = false),
…
)Without this fix the example mis‑communicates the component’s actual state to users.
📝 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.
| final bool _isBasicOpen = false; | |
| String? _basicSelectedValue; | |
| // Multiple select state | |
| final bool _isMultipleOpen = false; | |
| Set<String> _multipleSelectedValues = {}; | |
| // Disabled select state | |
| final bool _isDisabledOpen = false; | |
| String? _disabledSelectedValue; | |
| // Search select state | |
| final bool _isSearchOpen = false; | |
| String? _searchSelectedValue; | |
| late final TextEditingController _searchController; | |
| String _searchText = ''; | |
| final FocusNode _searchFocusNode = FocusNode(); | |
| int _keyboardSelectedIndex = -1; | |
| // Basic select state | |
| - final bool _isBasicOpen = false; | |
| + bool _isBasicOpen = false; // mutable | |
| String? _basicSelectedValue; | |
| // Multiple select state | |
| final bool _isMultipleOpen = false; | |
| Set<String> _multipleSelectedValues = {}; | |
| // Disabled select state | |
| final bool _isDisabledOpen = false; | |
| String? _disabledSelectedValue; | |
| // Search select state | |
| final bool _isSearchOpen = false; | |
| String? _searchSelectedValue; | |
| late final TextEditingController _searchController; | |
| String _searchText = ''; | |
| final FocusNode _searchFocusNode = FocusNode(); | |
| int _keyboardSelectedIndex = -1; | |
| // … later, in your build method … | |
| NakedSelect<String>( | |
| + onMenuOpen: () => setState(() => _isBasicOpen = true), | |
| + onMenuClose: () => setState(() => _isBasicOpen = false), | |
| // … other params … | |
| ) |
| Widget _buildSearchSelect(List<String> filteredOptions) { | ||
| return KeyboardListener( | ||
| focusNode: FocusNode(), | ||
| onKeyEvent: (event) { | ||
| if (event is KeyDownEvent) { | ||
| if (event.logicalKey == LogicalKeyboardKey.arrowDown) { | ||
| setState(() { | ||
| if (_keyboardSelectedIndex < filteredOptions.length - 1) { | ||
| _keyboardSelectedIndex++; | ||
| } | ||
| }); | ||
| } else if (event.logicalKey == LogicalKeyboardKey.arrowUp) { | ||
| setState(() { | ||
| if (_keyboardSelectedIndex > 0) { | ||
| _keyboardSelectedIndex--; | ||
| } | ||
| }); | ||
| } else if (event.logicalKey == LogicalKeyboardKey.enter && | ||
| _keyboardSelectedIndex >= 0 && |
There was a problem hiding this comment.
KeyboardListener allocates a new FocusNode on every build – potential leak & lost state
KeyboardListener(focusNode: FocusNode(), …) creates a fresh node each time
setState() is called, causing:
- GC churn (many short‑lived
FocusNodes), - lost keyboard focus between rebuilds,
- risk of memory leaks if the framework cannot dispose the node fast enough.
Create one node in initState() and dispose it in dispose():
-class _NakedSelectExampleState extends State<NakedSelectExample> {
+class _NakedSelectExampleState extends State<NakedSelectExample> {
+ late final FocusNode _keyboardListenerNode = FocusNode();
…
- return KeyboardListener(
- focusNode: FocusNode(),
+ return KeyboardListener(
+ focusNode: _keyboardListenerNode,
onKeyEvent: _handleSearchKeyEvent,
child: …
);
}
+
+ void _handleSearchKeyEvent(KeyEvent event) { … }
+
+ @override
+ void dispose() {
+ _keyboardListenerNode.dispose();
+ … // keep existing disposals
+ super.dispose();
+ }This preserves focus, avoids churn, and follows Flutter’s focus‑node
lifecycle guidelines.
| final _overlayController = OverlayPortalController(); | ||
| final _focusScopeNode = FocusScopeNode(); | ||
| late final _isMultipleSelection = | ||
| widget.allowMultiple && widget.selectedValues != null; | ||
| bool _isOpen = false; | ||
|
|
||
| // For type-ahead functionality | ||
| String _typeAheadBuffer = ''; | ||
| Timer? _typeAheadResetTimer; | ||
| final List<_SelectItemInfo<T>> _selectableItems = []; | ||
|
|
||
| @override | ||
| void dispose() { | ||
| _cancelTypeAheadTimer(); | ||
| _focusScopeNode.dispose(); | ||
| super.dispose(); | ||
| } |
There was a problem hiding this comment.
Overlay controller is never disposed
_overlayController owns a LayerLink and attaches to the overlay entry
tree. Although OverlayPortalController is cheap, it still allocates
resources that should be released.
@override
void dispose() {
_cancelTypeAheadTimer();
_focusScopeNode.dispose();
+ _overlayController.dispose();
super.dispose();
}Failing to dispose may leave hidden overlay entries or stray layer links
behind, affecting performance in long‑running widget trees.
📝 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.
| final _overlayController = OverlayPortalController(); | |
| final _focusScopeNode = FocusScopeNode(); | |
| late final _isMultipleSelection = | |
| widget.allowMultiple && widget.selectedValues != null; | |
| bool _isOpen = false; | |
| // For type-ahead functionality | |
| String _typeAheadBuffer = ''; | |
| Timer? _typeAheadResetTimer; | |
| final List<_SelectItemInfo<T>> _selectableItems = []; | |
| @override | |
| void dispose() { | |
| _cancelTypeAheadTimer(); | |
| _focusScopeNode.dispose(); | |
| super.dispose(); | |
| } | |
| @override | |
| void dispose() { | |
| _cancelTypeAheadTimer(); | |
| _focusScopeNode.dispose(); | |
| _overlayController.dispose(); | |
| super.dispose(); | |
| } |
| WidgetsBinding.instance.addPostFrameCallback((_) { | ||
| if (_isOpen) { | ||
| _overlayController.show(); | ||
| _selectableItems.clear(); | ||
| WidgetsBinding.instance.addPostFrameCallback((_) { | ||
| _focusScopeNode.requestFocus(); | ||
| }); | ||
| } else { | ||
| _overlayController.hide(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clear _selectableItems on close to avoid stale FocusNode references
When the menu closes _selectableItems keeps references to each item’s
FocusNode. If the menu is re‑built with a different set of items, the list
still contains obsolete nodes, which can lead to focus being requested on
widgets that no longer exist.
Add a clear in closeMenu():
void closeMenu() {
if (_isOpen) {
setState(() {
_isOpen = false;
handleOpenState();
+ _selectableItems.clear();
});
widget.onMenuClose?.call();
}
}This keeps the internal registry in sync with the visible menu.
📝 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.
| WidgetsBinding.instance.addPostFrameCallback((_) { | |
| if (_isOpen) { | |
| _overlayController.show(); | |
| _selectableItems.clear(); | |
| WidgetsBinding.instance.addPostFrameCallback((_) { | |
| _focusScopeNode.requestFocus(); | |
| }); | |
| } else { | |
| _overlayController.hide(); | |
| } | |
| }); | |
| void closeMenu() { | |
| if (_isOpen) { | |
| setState(() { | |
| _isOpen = false; | |
| handleOpenState(); | |
| _selectableItems.clear(); | |
| }); | |
| widget.onMenuClose?.call(); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/naked/lib/src/naked_accordion.dart (1)
1-172: Large block of commented‑out legacy code clutters the sourceLines 1‑172 contain an obsolete implementation (previous
NakedAccordionTypeAPI)
that’s now commented out. Keeping dead code in the file:
- Adds mental overhead for readers.
- Risks accidental resurrection during merges.
- Increases file size (affects IDE indexing).
Please delete the block or move it to version control history where it already lives.
packages/naked/example/lib/examples/naked_accordion_example.dart (2)
140-151: Minor: simplify single‑expand logic withcontroller.toggleInside
_BasicAccordion, you manually clear + open to emulate single‑item mode:if (_controller.values.contains(value)) { _controller.close(value); } else { _controller.clear(); _controller.open(value); }Consider adding a helper method to
AccordionController(e.g.openExclusive)
or simply:_controller ..clear() ..open(value);Keeps the example concise and showcases the controller API.
223-265: Performance note: hover/focus updates trigger full rebuildsEach hover/focus change calls
setState, rebuilding the entire accordion subtree.
For large lists this can be costly. Wrapping only the interactive header in a
StatefulBuilderor usingValueListenableBuilderfor per‑item state keeps the rest
of the list static.Not critical for a demo, but worth mentioning as a best practice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/naked/example/lib/examples/naked_accordion_example.dart(1 hunks)packages/naked/lib/src/naked_accordion.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test Min SDK
- GitHub Check: Test
- GitHub Check: production
🔇 Additional comments (1)
packages/naked/lib/src/naked_accordion.dart (1)
438-439: Default enabled flag may surprise consumersfinal bool enabled = widget.enabled ?? (accordion?.enabled ?? false);If an item is used outside an accordion,
accordionisnullandenableddefaults
tofalse, silently disabling the trigger. Defaulting totrueis usually safer:- final bool enabled = widget.enabled ?? (accordion?.enabled ?? false); + final bool enabled = widget.enabled ?? (accordion?.enabled ?? true);
| class AccordionController<T> { | ||
| final Set<T> values = {}; | ||
|
|
||
| void open(T value) { | ||
| values.add(value); | ||
| } | ||
|
|
||
| void close(T value) { | ||
| values.remove(value); | ||
| } | ||
|
|
||
| void toggle(T value) { | ||
| if (values.contains(value)) { | ||
| close(value); | ||
| } else { | ||
| open(value); | ||
| } | ||
| } | ||
|
|
||
| void clear() { | ||
| values.clear(); | ||
| } | ||
|
|
||
| void openAll(List<T> newValues) { | ||
| values.addAll(newValues); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rethink exposing a mutable values Set and the current equality override
AccordionController.values is a mutable collection that callers are expected to mutate.
Because the == operator and hashCode rely on the current content of that set, the
object’s hash/equality relationship is no longer stable once the instance is used as a
map key or placed in a Set. That can break look‑ups and lead to very hard‑to‑trace
bugs.
Two safer alternatives:
- Keep identity semantics (remove the
==/hashCodeoverrides) – the controller is
fundamentally a stateful object. - Make
valuesprivate and expose mutations through methods that broadcast change
notifications (e.g.ValueNotifier/ChangeNotifier).
Unless you have a compelling use‑case, option 1 keeps the API simple and avoids the foot
gun.
| @override | ||
| bool updateShouldNotify(_NakedAccordionScope<T> oldWidget) { | ||
| return controller != oldWidget.controller || | ||
| onTriggerPressed != oldWidget.onTriggerPressed; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
InheritedWidget never notifies on internal controller mutations
updateShouldNotify only compares the controller reference, not its internal
values. If the same controller instance changes (typical usage), dependants obtained
via _NakedAccordionScope.of(context) will NOT rebuild automatically.
You currently work around this by calling setState in the parent example code, but
consumers might forget and assume the scope is reactive.
Options:
bool updateShouldNotify(_NakedAccordionScope<T> oldWidget) {
- return controller != oldWidget.controller ||
- onTriggerPressed != oldWidget.onTriggerPressed;
+ return controller != oldWidget.controller ||
+ onTriggerPressed != oldWidget.onTriggerPressed ||
+ !setEquals(controller.values, oldWidget.controller.values);
}or expose a Listenable inside the controller and call dependOnInheritedWidgetOfExactType +
addListener.
Making this reactive will prevent subtle UI desyncs.
| late final FocusNode _focusNode = widget.focusNode ?? FocusNode(); | ||
|
|
||
| @override | ||
| void dispose() { | ||
| if (widget.focusNode == null) { | ||
| _focusNode.dispose(); | ||
| } | ||
| super.dispose(); | ||
| } | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| final accordionScope = _NakedAccordionScope.of<T>(context); | ||
| final itemScope = _NakedAccordionItemScope.of<T>(context); | ||
| final isExpanded = | ||
| accordionScope.controller.values.contains(itemScope.value); | ||
| final isInteractive = itemScope.enabled; | ||
|
|
||
| void handleTap() { | ||
| accordionScope.onTriggerPressed?.call(itemScope.value); | ||
| } | ||
|
|
||
| return Focus( | ||
| focusNode: widget.focusNode, | ||
| canRequestFocus: isInteractive, | ||
| onKeyEvent: (node, event) { | ||
| if (!isInteractive) return KeyEventResult.ignored; | ||
|
|
There was a problem hiding this comment.
FocusNode leak – use the node you create
_focusNode is created but never passed to the Focus widget; meanwhile Focus
creates another node that is never disposed. This leads to memory leaks and breaks
the dispose guard.
- return Focus(
- focusNode: widget.focusNode,
+ return Focus(
+ focusNode: _focusNode, // use the managed nodeAlso consider always using _focusNode and dropping the late‑init when the user
supplies one – it keeps the logic symmetric.
📝 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.
| late final FocusNode _focusNode = widget.focusNode ?? FocusNode(); | |
| @override | |
| void dispose() { | |
| if (widget.focusNode == null) { | |
| _focusNode.dispose(); | |
| } | |
| super.dispose(); | |
| } | |
| @override | |
| Widget build(BuildContext context) { | |
| final accordionScope = _NakedAccordionScope.of<T>(context); | |
| final itemScope = _NakedAccordionItemScope.of<T>(context); | |
| final isExpanded = | |
| accordionScope.controller.values.contains(itemScope.value); | |
| final isInteractive = itemScope.enabled; | |
| void handleTap() { | |
| accordionScope.onTriggerPressed?.call(itemScope.value); | |
| } | |
| return Focus( | |
| focusNode: widget.focusNode, | |
| canRequestFocus: isInteractive, | |
| onKeyEvent: (node, event) { | |
| if (!isInteractive) return KeyEventResult.ignored; | |
| return Focus( | |
| focusNode: _focusNode, // use the managed node | |
| canRequestFocus: isInteractive, | |
| onKeyEvent: (node, event) { | |
| if (!isInteractive) return KeyEventResult.ignored; | |
| … | |
| }, | |
| … | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/naked/lib/src/naked_textfield.dart (3)
658-699:⚠️ Potential issueCompile‑time error: each
casestill falls through – add explicitbreak/returnDart forbids implicit fall‑through between non‑empty
caseclauses, so this switch will not compile.
The same issue was raised in earlier feedback but the fix hasn’t landed yet.cursorOffset = Offset( iOSHorizontalOffset / MediaQuery.devicePixelRatioOf(context), 0, ); + break; // iOS @@ cursorOffset = Offset( iOSHorizontalOffset / MediaQuery.devicePixelRatioOf(context), 0, ); + break; // macOS @@ selectionColor = theme.colorScheme.primary.withOpacity(0.40); + break; // Android/Fuchsia @@ selectionColor = theme.colorScheme.primary.withOpacity(0.40); + break; // Linux/WindowsAdd
break;(orreturn) after every non‑emptycaseor merge the branches using pattern‑matching to restore compilability.
760-762: 🛠️ Refactor suggestionHard‑coded
restorationIdstill risks collisionsUsing the constant
'editable'causes a state‑restoration clash when multipleNakedTextFields sit on the same route. Derive it fromwidget.restorationId(when provided) or passnull.- restorationId: 'editable', + restorationId: widget.restorationId == null + ? null + : '${widget.restorationId}_editable',This mirrors Flutter’s own
TextFieldbehaviour and resolves the collision.
816-826:⚠️ Potential issuePressed‑state callback never reset on gesture cancel
onTapDownsetsonPressedState(true)andonSingleTapUpresets it, but a drag‑cancel (finger moves off the field) leaves the state stuck attrue.@override void onTapCancel() { super.onTapCancel(); _state.widget.onPressedState?.call(false); }Hooking
onTapCancelkeeps the pressed state accurate.
🧹 Nitpick comments (1)
packages/naked/lib/src/naked_textfield.dart (1)
618-621: DefaultTextStyleimposes black/grey styling – consider delegating to the callerHard‑coding
color: Colors.blackand a fixedfontSizecontradicts the “unstyled / blank canvas” promise and may break dark‑mode UIs.Options:
- Remove the explicit style and let
EditableTextuseDefaultTextStyle.of(context).- Expose a
textStyleparameter so consumers decide.- final TextStyle style = TextStyle( - color: widget.enabled ? Colors.black : Colors.grey, - fontSize: 16.0, - ); + final TextStyle style = + DefaultTextStyle.of(context).style.copyWith( + color: widget.enabled + ? DefaultTextStyle.of(context).style.color + : Colors.grey, + );This keeps the component truly headless.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
melos.yaml(2 hunks)packages/naked/example/lib/examples/textfield_example_page.dart(1 hunks)packages/naked/example/lib/main.dart(1 hunks)packages/naked/lib/src/naked_textfield.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- melos.yaml
- packages/naked/example/lib/examples/textfield_example_page.dart
- packages/naked/example/lib/main.dart
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Min SDK
- GitHub Check: production
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/naked/analysis_options.yaml (1)
27-35: Documenting custom conventions in comments
While these in‑file comments clearly outline naming and callback conventions, consider extracting them into a dedicated CONTRIBUTING.md or leveraging thecustom_lintpackage to programmatically enforce these rules. This makes them more discoverable and, when automated, prevents drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/mix/lib/src/core/element.dart(1 hunks)packages/mix/lib/src/core/utility.dart(1 hunks)packages/mix_generator/lib/src/core/metadata/base_metadata.dart(1 hunks)packages/naked/analysis_options.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/mix/lib/src/core/element.dart
- packages/mix_generator/lib/src/core/metadata/base_metadata.dart
- packages/mix/lib/src/core/utility.dart
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test
- GitHub Check: Test Min SDK
- GitHub Check: production
🔇 Additional comments (4)
packages/naked/analysis_options.yaml (4)
1-2: Extends flutter_lints for Flutter projects
Great choice to build onflutter_lints/flutter.yaml—this brings in Flutter‑specific best practices and ensures consistency across the package.
3-19: Comprehensive base rule set enabled
You've enabled a solid mix of style, safety, and correctness lints (e.g., always_declare_return_types, directives_ordering, unawaited_futures). This will help catch common mistakes early and keep the codebase consistent.
20-25: Enforcing Naked component conventions
The addition of camel_case_types, library_prefixes, and other state‑naming rules aligns well with the library’s API guidelines. These will reinforce the behavior‑first naming strategy across all widgets.
36-42: Clarify ignoring ofsort_unnamed_constructors_first
You’ve chosen to ignore the constructor ordering rule—please verify this aligns with your style guide. Consistent constructor ordering can aid readability, so you may want to demote it to warning or re-enable it later.
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 (2)
packages/naked/example/lib/examples/textfield_example_page.dart(1 hunks)packages/naked/lib/src/naked_button.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/naked/example/lib/examples/textfield_example_page.dart
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test Min SDK
- GitHub Check: Test
- GitHub Check: production
🔇 Additional comments (7)
packages/naked/lib/src/naked_button.dart (7)
180-187: LGTM! Good accessibility implementation.Excellent use of the Semantics widget to provide proper accessibility information, including button role, enabled state, loading hint, and custom label support. This ensures screen readers can properly interpret and announce the button to users.
166-178: LGTM! Clean logic for handling button interactions.Good implementation of the
isInteractiveflag that consolidates all states affecting button interactivity. The haptic feedback handling is also well-implemented with proper conditional logic.
187-207: LGTM! Excellent keyboard navigation support.The keyboard event handling for space and enter keys is well-implemented, allowing users to interact with the button using only keyboard input. This properly handles both key down and key up events, maintaining the visual pressed state between events.
208-226: LGTM! Appropriate mouse and gesture handling.Good implementation of mouse region and gesture detector with proper state management. The component correctly handles all gesture scenarios including tap down, tap up, and tap cancel with appropriate cursor changes.
155-164: LGTM! Proper focus node management.The code correctly handles focus node lifecycle by creating one internally when needed and disposing of it properly in the dispose method. This prevents memory leaks related to focus nodes.
5-58: LGTM! Excellent documentation and example.The class documentation is comprehensive and clear. The included example demonstrates the component's proper usage with state management, showing how to react to the various state callbacks. This will help developers understand how to use the component effectively.
67-130: LGTM! Well-documented properties with appropriate defaults.All properties have clear, descriptive documentation that explains their purpose and behavior. The default values are sensible and the optional parameters are appropriately marked.
This reverts commit 91c3f81.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/naked/lib/src/naked_button.dart (3)
231-234: Consider moving utility extension to a shared fileThis extension method is useful but might be needed by other components that handle keyboard events. Consider moving it to a shared utilities file to promote code reuse across the library.
- // In naked_button.dart - extension LogicalKeyboardKeyExtension on LogicalKeyboardKey { - bool get isSpaceOrEnter => - this == LogicalKeyboardKey.space || this == LogicalKeyboardKey.enter; - } + // In a new file: utilities/keyboard_extensions.dart + extension LogicalKeyboardKeyExtension on LogicalKeyboardKey { + bool get isSpaceOrEnter => + this == LogicalKeyboardKey.space || this == LogicalKeyboardKey.enter; + }Then import this extension where needed.
135-149: Consider adding autofocus propertyMany focusable Flutter widgets include an
autofocusproperty to specify whether the widget should receive focus when first displayed. Adding this would improve consistency with Flutter's standard widgets.const NakedButton({ super.key, required this.child, this.onPressed, this.onHoverState, this.onPressedState, this.onFocusState, this.loading = false, this.enabled = true, this.semanticLabel, this.cursor = SystemMouseCursors.click, this.enableHapticFeedback = true, this.focusNode, this.onEscapePressed, + this.autofocus = false, }); // Add to class properties: + /// Whether this button should be auto-focused when it first appears in the widget tree. + /// + /// When true, the button will be given focus when first inserted into the widget tree. + final bool autofocus; // Then in build method, update the Focus widget: child: Focus( focusNode: effectiveFocusNode, autofocus: widget.autofocus, // ... )
166-178: Consider making handleTap a class methodThe
handleTapmethod is defined inside the build method. While this is common in Flutter, extracting it to a class method would improve readability and testability, especially if the method becomes more complex in the future.- void handleTap() { - if (isInteractive) { - if (widget.enableHapticFeedback) { - HapticFeedback.lightImpact(); - } - widget.onPressed?.call(); - } - } + void _handleTap() { + if (!widget.enabled || widget.loading || widget.onPressed == null) return; + + if (widget.enableHapticFeedback) { + HapticFeedback.lightImpact(); + } + widget.onPressed?.call(); + } // Then update references to handleTap to use _handleTap
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/naked/lib/src/naked_button.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test
- GitHub Check: Test Min SDK
🔇 Additional comments (4)
packages/naked/lib/src/naked_button.dart (4)
59-153: Great implementation of a behavior-first button component!The component design excellently separates behavior from styling, providing comprehensive interaction handling (hover, press, focus) through callbacks while giving consumers complete control over appearance. The documentation and example are thorough and demonstrate the intended usage pattern clearly.
155-164: Proper focus node disposal in StatefulWidgetThe implementation correctly manages the lifecycle of the internally created focus node when one isn't provided. This avoids memory leaks that would occur if the focus node wasn't properly disposed.
188-204: Thorough keyboard event handlingThe implementation properly handles Space and Enter for activation and Escape for cancellation, making the button fully keyboard accessible. The state callbacks are properly triggered to reflect pressed states during keyboard interaction.
180-186: Comprehensive accessibility implementationThe Semantics widget properly marks this as a button, includes loading state hints, and ensures the component is fully accessible to screen readers. This is an excellent implementation of accessibility best practices.
Overview
This PR introduces "Naked", a behavior-first UI component library for Flutter that completely separates interaction logic from visual rendering. Naked components handle all complex interaction patterns, accessibility, and state management while giving developers complete freedom over styling and appearance.
Core Features
Key Components
NakedButton: Core button behavior with loading states and interaction trackingNakedCheckbox: Checkbox with checked, unchecked, and indeterminate statesNakedRadioButton&NakedRadioGroup: Exclusive selection controlsNakedAccordion: Expandable/collapsible content panelsNakedTabs: Tab-based navigation componentNakedSelect: Dropdown selection with positioningNakedMenu: Customizable dropdown menu with positioningNakedSlider: Customizable range sliderNakedAvatar: Flexible avatar componentImplementation Decisions
Callback-Based State Management
onHoverState,onFocusState, etc.) over builder functionsValueChanged<T>)Layered Implementation Structure
Core Utilities
NakedFocusManagerto handle complex keyboard focus requirementsNakedPortalfor rendering content in app overlays while maintaining contextNakedPositioningto handle optimal placement of elements relative to anchorsNo Style Mixins or Theme Integration
Property Naming Conventions
isprefix for boolean state (e.g.,isDisabled,isChecked)on{State}Statepattern (e.g.,onHoverState,onFocusState)Documentation & Examples
Impact & Use Cases
This package is ideal for:
Next Steps
Summary by CodeRabbit
New Features
Tests
Chores