feat(theme-selection): improve UI and smooth UX for theme choices #527
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces modular theme selection tiles in ThemeScreen with padding and styled ListTiles, adds ThemeIcons enum for icon mapping, and creates TileTitle and TileTrailing widgets. Selection triggers themeController.setTheme with the lowercase theme name. Titles use AppLocalizations. No existing public APIs changed; new public widgets and enum added. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant ThemeScreen
participant TileTitle
participant TileTrailing
participant ThemeIcons
participant ThemeController
User->>ThemeScreen: Open Themes
ThemeScreen->>ThemeIcons: Resolve icon by theme name
ThemeScreen->>TileTitle: Build(title, theme, isSelected)
ThemeScreen->>TileTrailing: Build(theme, isSelected)
User->>ThemeScreen: Tap a theme tile
activate ThemeScreen
ThemeScreen->>ThemeController: setTheme(theme.name.toLowerCase())
deactivate ThemeScreen
ThemeController-->>ThemeScreen: Notifies theme change
ThemeScreen-->>User: UI updates selected tile
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (4 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Poem
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🎉 Welcome @TheGoodUser!
We appreciate your contribution! 🚀 |
M4dhav
left a comment
M4dhav
left a comment
There was a problem hiding this comment.
Please do these refactoring changes
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
M4dhav
added
enhancement
|
Also please don't change the PR Template format like this, you are supposed to edit it in place |
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/themes/theme_screen.dart (1)
47-198: Consider extracting widgets and improving names.
Move builders into dedicated stateless widgets (ThemeTile, ThemeLeadingIcon, ThemeSelectionIndicator) for reuse/testability; aligns with prior feedback.
🧹 Nitpick comments (8)
lib/themes/theme_enum.dart (2)
7-16: Use strong typing instead of String for enum association.
Avoid string drift and simplify lookups by binding ThemeIcons to Themes.Apply:
-enum ThemeIcons { - classic('classic', Icons.diamond), - vintage('vintage', Icons.temple_buddhist_rounded), - forest('forest', Icons.forest), - cream('cream', Icons.nights_stay), - amber('amber', Icons.local_fire_department), - time('time', Icons.access_time_outlined); +enum ThemeIcons { + classic(Themes.classic, Icons.diamond), + vintage(Themes.vintage, Icons.temple_buddhist_rounded), + forest(Themes.forest, Icons.forest), + cream(Themes.cream, Icons.nights_stay), + amber(Themes.amber, Icons.local_fire_department), + time(Themes.time, Icons.access_time_outlined); - final String theme; + final Themes theme; final IconData icon; const ThemeIcons(this.theme, this.icon); }
7-12: Provide a helper/extension for icon lookup to avoid repeated scans.Add (outside this hunk):
extension ThemesX on Themes { IconData get icon => ThemeIcons.values.firstWhere((e) => e.theme == this).icon; }lib/themes/theme_screen.dart (6)
18-20: Localize user-facing strings.
Use AppLocalizations for AppBar title and the 'none' label.- appBar: AppBar( - title: const Text("Themes"), - ), + appBar: AppBar( + title: Text(AppLocalizations.of(context)!.themes), + ), ... - ? const Text("none") + ? Text(AppLocalizations.of(context)!.none)Also applies to: 25-25
193-196: Honor theme contrast for leading icon.
UseonPrimaryColorinstead of hardcoded white.- child: Icon( - iconData, size: 25, - color: Colors.white, - ), + child: Icon( + iconData, + size: 25, + color: theme.onPrimaryColor, + ),
73-77: ListTile color props conflict; align with Material tokens.
LetselectedTileColordrive background; avoidblack26default.- tileColor: isSelected ? theme.primaryColor : Colors.black26, - selected: isSelected, - selectedColor: theme.primaryColor, - selectedTileColor: theme.secondaryColor, + selected: isSelected, + tileColor: Colors.transparent, + selectedTileColor: theme.primaryColor, + selectedColor: theme.onPrimaryColor,
54-54: Remove no-op decoration.- decoration: BoxDecoration(),
23-23: Use const constructors for minor perf/readability.- padding: EdgeInsets.symmetric(horizontal: 8, vertical: 2), + padding: const EdgeInsets.symmetric(horizontal: 8, vertical: 2), ... - contentPadding: EdgeInsets.symmetric(vertical: 13, horizontal: 18), + contentPadding: const EdgeInsets.symmetric(vertical: 13, horizontal: 18), ... - padding: EdgeInsets.all(5), + padding: const EdgeInsets.all(5),Also applies to: 56-56, 157-157
178-185: Avoid per-build linear scans for icon mapping.
Cache a map once or use a typed enum helper (see enum comment) to cut repeated iterations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/themes/theme_enum.dart(1 hunks)lib/themes/theme_screen.dart(2 hunks)
🔇 Additional comments (3)
lib/themes/theme_enum.dart (2)
3-17: Enums extracted to a dedicated file — good separation.
8-8: Verify icon availability across your Flutter version. Icons.temple_buddhist_rounded is present in the Flutter Icons API but may be missing on older SDKs — confirm your minimum Flutter SDK/Material Icons font supports it or fall back to Icons.temple_buddhist or IconData(0xf039a, fontFamily: 'MaterialIcons'). (api.flutter.dev)lib/themes/theme_screen.dart (1)
107-115: , the spacing feature is fully available. No SizedBox workarounds needed.
|
Please also implement changes suggested by coderabbit |
Apologies for changing the PR template format. I’ll make sure to follow the guidelines and only edit in place going forward. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
lib/themes/theme_screen.dart (1)
40-46: Icon lookup robust and localized-title-safe.Using
firstWhere(..., orElse: ...)with the raw theme key avoids runtime throws when no match exists. Nice.
🧹 Nitpick comments (6)
lib/themes/theme_icon_enum.dart (1)
3-14: Optional: remove duplicatedthemestring; use enum.nameinstead.This avoids drift between the enum value and the stored string, and reduces API surface.
-enum ThemeIcons { - classic('classic', Icons.diamond), - vintage('vintage', Icons.temple_buddhist_rounded), - forest('forest', Icons.forest), - cream('cream', Icons.nights_stay), - amber('amber', Icons.local_fire_department), - time('time', Icons.access_time_outlined); - - final String theme; - final IconData icon; - const ThemeIcons(this.theme, this.icon); -} +enum ThemeIcons { + classic(Icons.diamond), + vintage(Icons.temple_buddhist_rounded), + forest(Icons.forest), + cream(Icons.nights_stay), + amber(Icons.local_fire_department), + time(Icons.access_time_outlined); + + final IconData icon; + const ThemeIcons(this.icon); +}Follow-up: update call sites to compare with
e.nameinstead ofe.theme. See suggested change in theme_screen.dart (Lines 40-45).lib/themes/theme_tile_trailing.dart (1)
15-39: Make decoration constants to reduce rebuild allocations.Const-qualify static shadows and colors.
- return Container( + return Container( height: size, width: size, decoration: BoxDecoration( - boxShadow: [ - BoxShadow( - color: Color.fromRGBO(50, 50, 93, 0.25), + boxShadow: const [ + BoxShadow( + color: Color.fromRGBO(50, 50, 93, 0.25), blurRadius: 27, spreadRadius: -5, offset: Offset(0, 13), ), - BoxShadow( - color: Color.fromRGBO(0, 0, 0, 0.3), + BoxShadow( + color: Color.fromRGBO(0, 0, 0, 0.3), blurRadius: 16, spreadRadius: -8, offset: Offset(0, 8), ), ], shape: BoxShape.circle, - color: Colors.white, + color: Colors.white, ), );lib/themes/theme_screen.dart (4)
83-88: Simplify tile colors; rely onselectedTileColor.No need to branch
tileColoronisSelectedwhenselectedTileColoris set.- tileColor: isSelected - ? theme.primaryColor - : Colors.black26, + tileColor: Colors.black26, selected: isSelected, - selectedColor: theme.primaryColor, + selectedColor: theme.primaryColor, selectedTileColor: theme.secondaryColor,
23-23: Use consts and remove no-op decoration.Minor allocation wins and cleanup.
- padding: EdgeInsets.symmetric(horizontal: 8, vertical: 2), + padding: const EdgeInsets.symmetric(horizontal: 8, vertical: 2), @@ - contentPadding: EdgeInsets.symmetric( + contentPadding: const EdgeInsets.symmetric( vertical: 13, horizontal: 18, ), @@ - padding: const EdgeInsets.all(12.0), + padding: const EdgeInsets.all(12.0), @@ - ), + ), @@ - decoration: BoxDecoration(), + // decoration removed; had no effectAlso applies to: 51-54, 68-68, 76-76, 47-50
20-21: Consider localizing literals.“Themes” and “none” are hardcoded; prefer AppLocalizations for consistency.
Also applies to: 24-26
40-46: If adopting enum.name, update comparison accordingly.Needed only if you apply the ThemeIcons refactor.
- final IconData iconData = ThemeIcons.values - .firstWhere( - (e) => e.theme == theme.name.toLowerCase(), - orElse: () => ThemeIcons.classic, - ) - .icon; + final IconData iconData = ThemeIcons.values + .firstWhere( + (e) => e.name == theme.name.toLowerCase(), + orElse: () => ThemeIcons.classic, + ) + .icon;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/themes/theme_icon_enum.dart(1 hunks)lib/themes/theme_screen.dart(2 hunks)lib/themes/theme_tile_title.dart(1 hunks)lib/themes/theme_tile_trailing.dart(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-11T21:08:07.631Z
Learnt from: M4dhav
PR: AOSSIE-Org/Resonate#527
File: lib/themes/theme_screen.dart:57-63
Timestamp: 2025-09-11T21:08:07.631Z
Learning: In Flutter, while BorderRadiusGeometry.circular exists, the borderRadius parameter in RoundedRectangleBorder specifically expects a BorderRadius object, so use BorderRadius.circular instead of BorderRadiusGeometry.circular for better type compatibility.
Applied to files:
lib/themes/theme_screen.dart
📚 Learning: 2025-09-11T21:09:29.353Z
Learnt from: M4dhav
PR: AOSSIE-Org/Resonate#527
File: lib/themes/theme_screen.dart:57-63
Timestamp: 2025-09-11T21:09:29.353Z
Learning: In Flutter, when you don't want a border on a widget, use BorderSide.none instead of creating a BorderSide with transparent color. BorderSide.none is more semantically correct, performant, and follows Flutter conventions.
Applied to files:
lib/themes/theme_screen.dart
🔇 Additional comments (1)
lib/themes/theme_icon_enum.dart (1)
5-9: Verify icon availability across your min Flutter SDK.Some symbols (e.g.,
Icons.temple_buddhist_rounded) depend on newer Material Symbols sets. Confirm they exist for your targeted Flutter/Dart versions to avoid compile-time errors.
|
Great work on the PR, thank you for your contribution! |
|
✅ PR Closed - Thank You, @TheGoodUser!
We appreciate your effort and look forward to more contributions from you! 🤝 |
Thanks a lot! Excited that my second PR got merged. Looking forward to contributing more! 🙌 |
2 participants
Description
Enhance theme selection UI for improved UX
Fixes #415
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Tested locally with updated UI components and verified changes across different screen sizes.
Recording.2025-09-12.015512.mp4
Checklist:
Maintainer Checklist
Summary by CodeRabbit
New Features
Refactor
Style