Refactor themes #3
Conversation
WalkthroughThe PR adds new theme classes (Yellow, Orange, Brown), updates MaterialTheme to use them, introduces Brown theme support, and adjusts theme mapping in ThemeCubit. It abstracts EmailListRepository into an interface with an implementation, wires it through DI (GetIt/Injectable and providers), adds a basic ThemeItem widget, and updates tests to use a mock. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App
participant GetIt as DI Container
participant RepoModule as RepositoryModule
participant UI as Widget/Bloc
participant Repo as EmailListRepositoryImpl
User->>App: Launch
App->>GetIt: init()
GetIt->>RepoModule: construct
GetIt->>GetIt: register factory EmailListRepository = RepoModule.provide()
UI->>GetIt: resolve EmailListRepository
GetIt-->>UI: EmailListRepositoryImpl
UI->>Repo: loadData()
Repo-->>UI: Future<List<Email>>
sequenceDiagram
autonumber
actor User
participant ThemeCubit
participant MaterialTheme
participant ThemeData
User->>ThemeCubit: select AppTheme.darkMint
ThemeCubit->>MaterialTheme: brownDark()
MaterialTheme-->>ThemeCubit: ThemeData (Brown dark scheme)
ThemeCubit-->>ThemeData: emit/update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
lib/theme/brown/brown_theme.dart (1)
3-309: Well-structured theme implementation.The BrownTheme class provides a comprehensive set of color schemes for different brightness levels and contrast requirements. The code is well-organized and follows a consistent pattern for defining color schemes.
One suggestion to consider for future improvements would be to extract common color values into constants to reduce duplication, especially for colors that appear in multiple schemes.
lib/theme/orange/orange_theme.dart (2)
3-7: Inconsistent class design compared to BrownTheme.Unlike BrownTheme, OrangeTheme accepts a TextTheme in its constructor and stores it as a member variable, but it's not used in any of the static methods. Consider either:
- Making the methods non-static and using the textTheme member variable, or
- Removing the constructor and textTheme field to make it consistent with BrownTheme
-class OrangeTheme { - OrangeTheme(this.textTheme); - - final TextTheme textTheme; +class OrangeTheme {
8-313: Inconsistent method naming convention.The method naming convention in OrangeTheme (e.g.,
orangeLightScheme()) differs from BrownTheme (e.g.,lightScheme()). This inconsistency can make the API less intuitive. Consider standardizing the naming convention across all theme classes for better maintainability.For example, rename methods to match BrownTheme's convention:
-static ColorScheme orangeLightScheme() { +static ColorScheme lightScheme() {And similarly for the other methods.
lib/theme/yellow/yellow_theme.dart (3)
3-53: Consider reducing duplication or adding documentation.The
yellowLightScheme()function is quite verbose. You could reduce duplication across similar methods (e.g. grouping shared values, extracting a helper) or add brief doc comments explaining the purpose of specific color sets to improve maintainability.
157-206: Optional doc comments could improve clarity.The
yellowDarkScheme()function seems correct. A short explanation describing the intent (e.g., usage scenario, recommended background color) can help newer developers.
208-257: Possible candidate for reduced repetition or a single parameterized function.
yellowDarkMediumContrastScheme()replicates many values from other variants, which can lead to duplication. Consider a pattern to share common color definitions across your theme variants.lib/theme/style.dart (1)
36-36: Possible redundant comment.The
//BrownThemecomment may be superfluous since the method names themselves convey that these are Brown theme variants. Consider removing it if it doesn’t add clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
lib/app/app.dart(1 hunks)lib/bloc/theme/theme_cubit.dart(1 hunks)lib/di/di_initializer.config.dart(3 hunks)lib/di/di_repository_module.dart(2 hunks)lib/features/settings/theme_item.dart(1 hunks)lib/repository/email_list_repository.dart(1 hunks)lib/theme/brown/brown_theme.dart(1 hunks)lib/theme/color_family.dart(1 hunks)lib/theme/extended_color.dart(1 hunks)lib/theme/orange/orange_theme.dart(1 hunks)lib/theme/style.dart(1 hunks)lib/theme/yellow/yellow_theme.dart(1 hunks)pubspec.yaml(1 hunks)test/repository/email_list_repository_test.dart(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
test/repository/email_list_repository_test.dart
[error] 34-34: MissingStubError: 'loadData'. No stub was found which matches the arguments of this method call: loadData(). Add a stub for this method using Mockito's 'when' API, or generate the MockEmailListRepository mock with the @GenerateNiceMocks annotation.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (25)
pubspec.yaml (1)
98-98: No effective change in Flutter intl configurationThe line appears to be unchanged in terms of functionality, as it remains
enabled: truebefore and after the changes.lib/theme/color_family.dart (1)
5-25: Good implementation of ColorFamily class with proper value equalityThe
ColorFamilyclass is well-structured and follows good practices:
- Uses
Equatablefor value-based equality- Has properly documented required parameters
- Follows immutable pattern with final fields
- Includes all properties in the props list for equality comparison
This will provide a strong foundation for your theme system refactoring, enabling consistent color definitions across different themes.
lib/di/di_repository_module.dart (2)
3-3: Added import for EmailListRepositoryThe import statement correctly brings in the necessary EmailListRepository class.
14-16:✅ Verification successful
Properly registered EmailListRepository in DI container
The implementation correctly uses the
@factoryMethodannotation to register theEmailListRepositoryImplas the implementation forEmailListRepositoryinterface. This follows dependency injection best practices by:
- Allowing the interface to be injected rather than the concrete implementation
- Using the factory method pattern for creating instances
- Maintaining consistency with other repository registrations in this file
🏁 Script executed:
#!/bin/bash # Check if EmailListRepository is properly implemented as an abstract class # with EmailListRepositoryImpl as its concrete implementation echo "Checking EmailListRepository implementation:" rg -A 10 "abstract class EmailListRepository" echo "Checking EmailListRepositoryImpl implementation:" rg -A 10 "class EmailListRepositoryImpl implements EmailListRepository"Length of output: 1647
DI Registration Verified: The EmailListRepository is Registered Correctly
- The interface and implementation are defined as expected in
lib/repository/email_list_repository.dart:
EmailListRepositoryis correctly declared as an abstract class.EmailListRepositoryImplproperly implements the interface.- The DI registration in
lib/di/di_repository_module.dartuses the@factoryMethodannotation to provideEmailListRepositoryImplfor theEmailListRepositoryinterface, following dependency injection best practices.The verification confirms that the current implementation correctly follows the intended DI pattern. No further changes are needed.
lib/bloc/theme/theme_cubit.dart (2)
12-12: Theme mapping updated for lightMintThe theme mapping for
AppTheme.lightMinthas been changed from usingyellowLightMediumContrast()tobrownLight(), which aligns with the new theme refactoring approach. This change is consistent with the PR objective of refactoring themes.
15-16: Theme mappings updated for darkMint and experimental themesThe theme mappings have been appropriately updated:
AppTheme.darkMintnow usesbrownDark()instead ofyellowDarkMediumContrast()AppTheme.experimentalnow usesyellowDark()instead ofyellowDarkMediumContrast()These changes are part of the broader theme system refactoring which appears to organize color schemes into more specific theme classes.
lib/theme/extended_color.dart (1)
6-37: Well-structured ExtendedColor class for theme managementThe newly added
ExtendedColorclass provides a comprehensive structure for managing colors across different theme modes and contrast levels. It appropriately extendsEquatablefor value equality comparison and includes all necessary properties for the theme system refactoring.The class design follows good practices:
- Clear constructor with required parameters
- Proper field declarations
- Overridden props getter for equality comparison
This is a good addition to the theme system that will help manage colors consistently.
lib/di/di_initializer.config.dart (4)
24-25: Added import for EmailListRepositoryThe import for
EmailListRepositoryhas been properly added to support the dependency injection registration.
48-48: Repositioned DIDataModule declarationThe declaration of
dIDataModulehas been moved but maintains its functionality.
51-53: Added dependency injection for EmailListRepositoryThe factory registration for
EmailListRepositoryhas been added to the dependency injection container. This aligns with the broader refactoring to move from context-based repository providers to direct retrieval via dependency injection.
74-74: Repositioned DIDataModule class declarationThe class declaration for
_$DIDataModulehas been repositioned without functional changes.lib/repository/email_list_repository.dart (2)
6-8: Good use of abstraction to improve design.The introduction of an abstract
EmailListRepositoryclass establishes a clear contract through theloadData()method, which is a good application of the dependency inversion principle. This abstraction will make it easier to test and potentially implement alternative data sources in the future.
10-17: Implementation looks good.The concrete implementation
EmailListRepositoryImplproperly implements the interface and maintains the original functionality. This refactoring allows for better separation of concerns and improves testability.lib/app/app.dart (2)
34-35: DI approach improved with direct container retrieval.Good refactoring from context-based repository retrieval to direct dependency injection with
diContainer.get<EmailListRepository>(). This approach is more explicit and removes the dependency on the BuildContext for retrieving repositories.
41-42: Consistent DI pattern application.The same container-based DI approach is correctly applied here for the LaunchesRepository, maintaining consistency throughout the codebase.
lib/theme/yellow/yellow_theme.dart (3)
55-104: Looks good overall.The
yellowLightMediumContrastScheme()method follows the same structure as the light scheme. No issues spotted; contrast values appear logically consistent. Consider adding doc comments for clarity.
106-155: All good.The
yellowLightHighContrastScheme()is properly defined, with no evident errors. This keeps the code style consistent.
259-309: Implementation is consistent with the rest.The
yellowDarkHighContrastScheme()approach is aligned with the other theme definitions. No further issues noted.lib/theme/style.dart (7)
2-5: Imports look appropriate.These newly added imports correctly reference the new theme files. No concerns here.
13-13: Straightforward usage of the new theme method.
yellowLight()correctly delegates toYellowTheme.yellowLightScheme(). No issues found.
17-17: Proper dark scheme usage.
yellowDark()callsYellowTheme.yellowDarkScheme(). This follows the expected pattern.
21-21: Implementation aligns with new OrangeTheme.
orangeLight()correctly referencesOrangeTheme.orangeLightScheme(). No concerns.
25-25: Dark variant usage is consistent.
orangeDark()referencesOrangeTheme.orangeDarkScheme()in a similar manner. Looks good.
28-29: Brown dark theme logic is fine.
brownDark()simply delegates toBrownTheme.darkScheme(). No issues spotted.
32-33: Brown light theme logic is fine.
brownLight()callsBrownTheme.lightScheme(). This matches the established pattern.
| class ThemeItem extends StatelessWidget { | ||
| const ThemeItem({super.key}); | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| return const Placeholder(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
ThemeItem widget is incomplete and needs implementation details
The current implementation uses a Placeholder widget, which is only meant for development and not for production use. For a proper theme item widget in a settings screen, you should:
- Include properties to represent theme data (name, colors, etc.)
- Implement a visual preview of the theme
- Add selection mechanism (typically with a radio button or highlight)
- Connect to the theme system for actual theme switching
Consider implementing a more complete version:
class ThemeItem extends StatelessWidget {
- const ThemeItem({super.key});
+ const ThemeItem({
+ super.key,
+ required this.themeName,
+ required this.isSelected,
+ required this.onSelect,
+ required this.themeColor,
+ });
+
+ final String themeName;
+ final bool isSelected;
+ final VoidCallback onSelect;
+ final Color themeColor;
@override
Widget build(BuildContext context) {
- return const Placeholder();
+ return InkWell(
+ onTap: onSelect,
+ child: Container(
+ padding: const EdgeInsets.all(16),
+ decoration: BoxDecoration(
+ border: Border.all(
+ color: isSelected ? themeColor : Colors.grey.shade300,
+ width: isSelected ? 2 : 1,
+ ),
+ borderRadius: BorderRadius.circular(8),
+ ),
+ child: Row(
+ children: [
+ Container(
+ width: 24,
+ height: 24,
+ decoration: BoxDecoration(
+ color: themeColor,
+ shape: BoxShape.circle,
+ ),
+ ),
+ const SizedBox(width: 16),
+ Text(themeName),
+ const Spacer(),
+ if (isSelected)
+ Icon(Icons.check_circle, color: themeColor),
+ ],
+ ),
+ ),
+ );
}
}📝 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.
| class ThemeItem extends StatelessWidget { | |
| const ThemeItem({super.key}); | |
| @override | |
| Widget build(BuildContext context) { | |
| return const Placeholder(); | |
| } | |
| } | |
| import 'package:flutter/material.dart'; | |
| class ThemeItem extends StatelessWidget { | |
| const ThemeItem({ | |
| super.key, | |
| required this.themeName, | |
| required this.isSelected, | |
| required this.onSelect, | |
| required this.themeColor, | |
| }); | |
| final String themeName; | |
| final bool isSelected; | |
| final VoidCallback onSelect; | |
| final Color themeColor; | |
| @override | |
| Widget build(BuildContext context) { | |
| return InkWell( | |
| onTap: onSelect, | |
| child: Container( | |
| padding: const EdgeInsets.all(16), | |
| decoration: BoxDecoration( | |
| border: Border.all( | |
| color: isSelected ? themeColor : Colors.grey.shade300, | |
| width: isSelected ? 2 : 1, | |
| ), | |
| borderRadius: BorderRadius.circular(8), | |
| ), | |
| child: Row( | |
| children: [ | |
| Container( | |
| width: 24, | |
| height: 24, | |
| decoration: BoxDecoration( | |
| color: themeColor, | |
| shape: BoxShape.circle, | |
| ), | |
| ), | |
| const SizedBox(width: 16), | |
| Text(themeName), | |
| const Spacer(), | |
| if (isSelected) Icon(Icons.check_circle, color: themeColor), | |
| ], | |
| ), | |
| ), | |
| ); | |
| } | |
| } |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
# Conflicts: # lib/app/app.dart # lib/di/di_initializer.config.dart
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/di/di_repository_module.dart (2)
18-20: Consider singleton lifecycle for repositoryIf EmailListRepository holds caches/state, prefer a lazy singleton to prevent multiple instances.
Optional diff:
- @factoryMethod - EmailListRepository provideEmailListRepository() => EmailListRepositoryImpl(); + @lazySingleton + EmailListRepository provideEmailListRepository() => EmailListRepositoryImpl();
15-16: Method name “provideAccidentsRepository” is misleadingIt returns ThemeRepository; rename for clarity and regenerate DI.
Suggested diff:
- ThemeRepository provideAccidentsRepository(ThemeStorage themeStorage) => + ThemeRepository provideThemeRepository(ThemeStorage themeStorage) => ThemeRepositoryImpl(themeStorage);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/bloc/theme/theme_cubit.dart(1 hunks)lib/di/app_repository_providers.dart(1 hunks)lib/di/di_initializer.config.dart(3 hunks)lib/di/di_repository_module.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/bloc/theme/theme_cubit.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Flutter Integration Tests (31)
- GitHub Check: App
🔇 Additional comments (1)
lib/di/di_initializer.config.dart (1)
32-33: Generated wiring looks correct; avoid manual editsEmailListRepository is registered via repositoryModule; wrapper _$RepositoryModule is present. If you switch to @lazySingleton or rename provideThemeRepository, re-run code generation.
Also applies to: 57-57, 61-63, 92-93
| create: (context) => EmailListRepositoryImpl(), | ||
| ), |
There was a problem hiding this comment.
Unify DI source to avoid duplicate EmailListRepository instances
Directly instantiating EmailListRepositoryImpl here while also registering EmailListRepository in GetIt can yield two separate instances and divergent state/caches. Fetch it from GetIt like the other repositories.
Apply this diff:
RepositoryProvider<EmailListRepository>(
- create: (context) => EmailListRepositoryImpl(),
+ create: (context) => diContainer.get<EmailListRepository>(),
),📝 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.
| create: (context) => EmailListRepositoryImpl(), | |
| ), | |
| RepositoryProvider<EmailListRepository>( | |
| create: (context) => diContainer.get<EmailListRepository>(), | |
| ), |
🤖 Prompt for AI Agents
In lib/di/app_repository_providers.dart around lines 15 to 16, the provider is
directly instantiating EmailListRepositoryImpl which can create a separate
instance from the one registered in GetIt; replace the direct construction with
retrieving the singleton from GetIt (e.g., use
GetIt.instance.get<EmailListRepository>() or the project's getIt helper) so the
provider returns the same registered EmailListRepository; also add the GetIt
import if missing and remove any unused direct-impl import.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3 +/- ##
==========================================
- Coverage 84.90% 83.83% -1.08%
==========================================
Files 150 155 +5
Lines 3194 3205 +11
==========================================
- Hits 2712 2687 -25
- Misses 482 518 +36 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
New Features
Style