Conversation
- Created shared theme system in docpilot_core - Implemented comprehensive text styles (TextStyle) with all variants - Added ThemeHelper for centralized theme access - Integrated theme system in docpilot_doctor and docpilot_patient - Added icon assets to docpilot_doctor - Updated palette definitions for consistency across apps Addresses #17
WalkthroughThe PR centralizes the design system by introducing DocPilotTextStyles to docpilot_core and expanding ThemeHelper with standardized color constants, icon sizes, and border widths. Both doctor and patient apps migrate to use ThemeHelper-sourced colors via Palette and add build runner/flutter_gen configuration for code generation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docpilot_doctor/lib/core/theme/palette.dart (1)
50-55: Disabled text contrast fails WCAG AA.Verified: textDisabled (0xFF8F9098 on 0xFFFFFFFF background) achieves only 3.18:1 contrast ratio, falling short of the 4.5:1 minimum for WCAG AA compliance. Consider lightening the disabled text color or darkening the background to meet accessibility standards.
🧹 Nitpick comments (8)
docpilot_core/lib/src/theme/theme_helper.dart (1)
151-167: Progress indicator theme - one tiny nit on const.The theme configuration looks good overall, but line 158 should use
const EdgeInsets.all(2.0)since it's a compile-time constant.- circularTrackPadding: EdgeInsets.all(2.0), + circularTrackPadding: const EdgeInsets.all(2.0),docpilot_doctor/lib/core/theme/app_theme.dart (1)
184-193: Theme simplified to delegate to ThemeHelper - but the copyWith is a no-op.The refactor to rely entirely on ThemeHelper.createAppTheme is clean and aligns with centralized theming. However, line 192 calls
baseTheme.copyWith()with no arguments, which effectively does nothing.You can simplify this further:
static ThemeData get lightTheme { // Get base theme from ThemeHelper with doctor app primary color final baseTheme = ThemeHelper.createAppTheme( primaryColor: Palette.primary, fontFamily: fontFamily, ); - // Customize with doctor app specific colors - return baseTheme.copyWith(); + return baseTheme; }Or if you anticipate adding customizations later, add a comment explaining the structure:
// Return base theme (doctor-specific customizations can be added here if needed) return baseTheme;docpilot_core/lib/src/theme/text_style.dart (5)
16-23: Unify text colors with ThemeHelper; avoid hex drift.All these styles hard-code 0xFF1F1F1F while helpers use 0xFF1F2024. Pipe base text color through ThemeHelper (e.g., ThemeHelper.neutralDarkDarkest) to keep one source of truth and simplify dark‑mode later.
Example (apply similarly across all styles):
- static const TextStyle h1 = TextStyle( + static const TextStyle h1 = TextStyle( fontFamily: _fontFamily, fontSize: 24, fontWeight: FontWeight.w800, // Extra Bold height: 1.0, // 100% line height letterSpacing: 0.24, - color: Color(0xFF1F1F1F), + color: ThemeHelper.neutralDarkDarkest, );Note: import ThemeHelper from core internals to avoid circular export. Keep it consistent across h1–h5, bodyXL–XS, actionL–S, captionM.
Also applies to: 27-34, 39-45, 49-55, 59-65, 71-77, 81-87, 91-97, 101-108, 112-119, 125-131, 135-141, 145-151, 159-165
169-199: Make color helpers theme-aware and de-duplicate hex.These helpers lock in hex values. Consider deriving from ThemeHelper or a ThemeData ColorScheme to adapt to light/dark and reduce duplication.
Minimal option:
- static TextStyle withDarkColor(TextStyle style) { - return style.copyWith(color: const Color(0xFF1F2024)); - } + static TextStyle withDarkColor(TextStyle style) { + return style.copyWith(color: ThemeHelper.neutralDarkDarkest); + }Theme-aware option:
+ static TextStyle withColorFromTheme( + BuildContext context, + TextStyle style, + Color Function(ColorScheme s) pick, + ) { + final scheme = Theme.of(context).colorScheme; + return style.copyWith(color: pick(scheme)); + }
14-24: Heading line-height at 1.0 can feel tight and clip with scale.Consider removing explicit height (let font metrics drive) or using a slightly looser ratio (e.g., 1.15–1.2) for multi-line headings. Helps with large text scaling.
Also applies to: 25-35, 36-46, 47-56, 57-66
155-165: Caption uppercase: provide an API, not a comment.Add an opt-in helper or Text widget wrapper to transform to uppercase to avoid caller-by-caller string munging.
Example:
class UppercaseText extends StatelessWidget { final String data; final TextStyle? style; const UppercaseText(this.data, {super.key, this.style}); @override Widget build(BuildContext context) => Text(data.toUpperCase(), style: style ?? DocPilotTextStyles.captionM); }
3-11: Naming alignment with design tokens.PR summary mentions display/headline/title/body/label. File exposes h1–h5, body*, action*, caption*. Consider alias getters or mapping docs to reduce cognitive load.
Example aliases:
static const TextStyle headline1 = h1; static const TextStyle labelM = actionM;docpilot_doctor/lib/core/theme/palette.dart (1)
12-18: Consider centralizing primary shades too.Primary* are still hex here. If core owns tokens, mirror them in ThemeHelper (e.g., ThemeHelper.primaryDarkest, …) and reference here to prevent drift between apps.
Example:
- static const Color primaryDarkest = Color(0xFF006FFD); + static const Color primaryDarkest = ThemeHelper.primaryDarkest;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
docpilot_doctor/assets/icons/calendar.svgis excluded by!**/*.svgdocpilot_doctor/assets/icons/calendar_selected.svgis excluded by!**/*.svgdocpilot_doctor/assets/icons/home.svgis excluded by!**/*.svgdocpilot_doctor/assets/icons/home_selected.svgis excluded by!**/*.svgdocpilot_doctor/assets/icons/patients.svgis excluded by!**/*.svgdocpilot_doctor/assets/icons/patients_selected.svgis excluded by!**/*.svgdocpilot_doctor/assets/icons/prescribe.svgis excluded by!**/*.svgdocpilot_doctor/assets/icons/prescribe_selected.svgis excluded by!**/*.svgdocpilot_doctor/pubspec.lockis excluded by!**/*.lockdocpilot_patient/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
docpilot_core/lib/docpilot_core.dart(1 hunks)docpilot_core/lib/src/theme/text_style.dart(1 hunks)docpilot_core/lib/src/theme/theme_helper.dart(9 hunks)docpilot_doctor/.gitignore(1 hunks)docpilot_doctor/lib/core/theme/app_theme.dart(1 hunks)docpilot_doctor/lib/core/theme/palette.dart(2 hunks)docpilot_doctor/lib/main.dart(2 hunks)docpilot_doctor/pubspec.yaml(2 hunks)docpilot_patient/.gitignore(1 hunks)docpilot_patient/lib/core/theme/palette.dart(2 hunks)docpilot_patient/pubspec.yaml(2 hunks)
🔇 Additional comments (21)
docpilot_patient/.gitignore (1)
46-49: Clean addition of generated file patterns.The three ignore patterns correctly exclude common Dart-generated artifacts (flutter_gen, build_runner, freezed). Standard and appropriate for the tooling being integrated.
docpilot_doctor/.gitignore (1)
47-49: LGTM! Solid additions for generated code.These ignore patterns are exactly what you need for the new code generation tooling (flutter_gen, build_runner) added in the pubspec. Keeps the repo clean from generated artifacts.
docpilot_patient/pubspec.yaml (2)
14-16: Dependencies look good, version alignment is solid.The flutter_svg and solar_icons versions match the doctor app setup. Nice consistency across modules.
49-53: Review comment is incorrect—assets are already properly declared.The pubspec.yaml already contains an
assets:block in the flutter section. The Inter font family with all 9 font weights (100-900) is properly declared with corresponding asset paths. No changes needed.Likely an incorrect or invalid review comment.
docpilot_doctor/pubspec.yaml (3)
14-16: Dependencies looking fresh.flutter_svg and solar_icons versions are consistent with the patient app. Nice work keeping versions aligned across the monorepo.
51-55: Flutter Gen config is clean.Output path and SVG integration look good for generating type-safe asset references.
27-28: Assets directory verified ✓The
assets/icons/directory exists and contains all navigation icons as expected—calendar, home, patients, and prescribe (with selected state variants). The pubspec.yaml declaration is correct.docpilot_core/lib/src/theme/theme_helper.dart (8)
2-2: Text style integration looks good.Importing the new text_style module to use DocPilotTextStyles in the theme configuration below.
22-49: Comprehensive color system - nicely organized.The neutral/success/warning/error color palettes are well-structured and follow a clear naming convention (darkest → lightest). This centralization will make theming much more consistent across apps.
65-73: Border width helper logic is clean.Returns thick border for error/focus states, medium otherwise. Straightforward and correct.
76-79: Text color helper looks solid.Uses theme colors appropriately for enabled/disabled states.
109-122: Filled button theme configured nicely.Primary color background with white foreground, proper spacing and border radius. The elevation: 0 gives it that modern flat look.
124-136: Outlined button theme looks correct.Consistent spacing and border radius with the filled variant, proper use of primary color for foreground and border.
138-149: Text button theme is on point.Lighter padding than filled/outlined buttons makes sense for tertiary actions.
178-192: Navigation bar theme integrated with DocPilotTextStyles.The dynamic text style switching based on selected state is solid. Using DocPilotTextStyles.actionM for selected items and bodyXS for unselected gives good visual hierarchy.
docpilot_doctor/lib/main.dart (2)
4-4: Theme import added cleanly.Bringing in the centralized theme configuration.
18-18: Theme application looks perfect.Wiring up AppTheme.lightTheme to the MaterialApp. Nice and clean integration.
docpilot_core/lib/docpilot_core.dart (1)
37-38: Text style utilities now public - nice.Exporting the new text_style module makes DocPilotTextStyles available across both apps. Clean API extension.
docpilot_patient/lib/core/theme/palette.dart (2)
1-1: Core theme integration added.Importing docpilot_core to access ThemeHelper color constants.
20-46: Color consolidation looks excellent.All neutral/success/warning/error colors now sourced from ThemeHelper, establishing a single source of truth. Patient-specific primary colors (purple/pink) remain local, which makes sense. This pattern keeps shared colors centralized while preserving app-specific brand colors.
docpilot_doctor/lib/core/theme/palette.dart (1)
1-1: Nice move to ThemeHelper.Swapping neutrals and semantic colors to ThemeHelper tightens consistency and reduces duplication. Looks good.
Also applies to: 20-25, 27-32, 34-37, 39-41, 44-46
|
🎊 Well done, @xkaper001! Your PR has been merged into the project! 🚀 Thank you for your hard work and dedication! Contributors like you make open source truly special. Keep being awesome! 💫🙏 |
1 participant
Overview
This PR implements a comprehensive theme system and shared text styles as foundational work for creating button and navigation components.
Changes Made
Shared Core Theme System (
docpilot_core)TextStyleclass with comprehensive text style variants:ThemeHelperfor centralized theme access across all appsdocpilot_core.dartDocPilot Doctor App
AppThemeto useThemeHelperflutter_svgdependency for icon renderingDocPilot Patient App
Technical Details
docpilot_coreRelated Issue
Closes #17
Testing
Next Steps
This PR sets the foundation for:
Screenshots
(Theme system and text styles are foundational - visual components will be demonstrated in subsequent PRs)
Checklist
Summary by CodeRabbit
New Features
Chores