-
Notifications
You must be signed in to change notification settings - Fork 14
feat(ui): streamline component layouts and implement centralized theme-based spacing #162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…e-based spacing Simplify complex UI structures by removing tab-based interfaces in favor of scrollable layouts. Introduce theme extension system for consistent page-level padding across the application. Consolidate inline component implementations to reduce architectural complexity while maintaining visual consistency.
Summary of ChangesHello @ahmet-cetinkaya, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refines the application's user interface by implementing a centralized, theme-based spacing system for consistent page layouts. It streamlines navigation by replacing tab-based interfaces with more flexible scrollable designs and consolidates UI logic by extracting shared components and standardizing existing ones. These changes aim to improve visual consistency, reduce code complexity, and enhance maintainability across various features including app usages, calendar, habits, notes, settings, sync, tags, and tasks. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Reviewer's GuideIntroduces a centralized theme-based page padding system via a ThemeExtension and refactors multiple feature pages to use it, while simplifying several UI layouts (notably tag details, tags list, app usage, sync, and settings) away from tabbed/over-padded structures into scrollable, section-based views with standardized headers and spacing. Sequence diagram for page-level padding resolution via theme extensionsequenceDiagram
participant User
participant Widget as FeaturePageState
participant Ctx as BuildContext
participant Theme as ThemeData
participant Ext as PagePaddingTheme
participant AppTheme
User->>Widget: navigate to page
Widget->>Ctx: build(context)
Widget->>Ctx: pageBodyPadding
activate Ctx
Ctx->>Theme: Theme.of(context)
activate Theme
Theme-->>Ctx: themeData
Ctx->>Theme: extension<PagePaddingTheme>()
Theme-->>Ctx: Ext
alt PagePaddingTheme present
Ctx->>Ctx: check MediaQuery.sizeOf(context).width
Ctx->>AppTheme: read screenSmall
AppTheme-->>Ctx: screenSmall
alt width <= screenSmall
Ctx-->>Widget: EdgeInsets.symmetric(horizontal: AppTheme.sizeSmall, vertical: Ext.vertical)
else width > screenSmall
Ctx-->>Widget: EdgeInsets.symmetric(horizontal: Ext.horizontal, vertical: Ext.vertical)
end
else extension missing
Ctx-->>Widget: EdgeInsets.zero
end
deactivate Ctx
Widget->>Widget: build layout with Padding(padding: pageBodyPadding)
Widget-->>User: rendered page with consistent spacing
Class diagram for centralized page padding theme and consumersclassDiagram
class ThemeDataBuilder {
+ThemeData buildThemeData()
-ThemeData _createThemeData()
-PagePaddingTheme _buildPagePaddingTheme()
}
class PagePaddingTheme {
+double horizontal
+double vertical
+PagePaddingTheme(horizontal, vertical)
+EdgeInsets padding
+PagePaddingTheme copyWith(horizontal, vertical)
+PagePaddingTheme lerp(other, t)
+double lerpDouble(a, b, t)
}
class BuildContext {
}
class PagePaddingContext {
+EdgeInsets pageBodyPadding
}
class AppTheme {
+double screenSmall
+double sizeSmall
+ThemeData themeData
}
class TagDetailsPage {
+String route
}
class _TagDetailsPageState {
-ITranslationService _translationService
-IThemeService _themeService
+Widget build(context)
-void _goBack()
}
class TagStatisticsView {
+String tagId
+TagStatisticsView(tagId)
}
class _TagStatisticsViewState {
-ITranslationService _translationService
-IThemeService _themeService
-GlobalKey barChartKey
-DateFilterSetting dateFilterSetting
-DateTime startDate
-DateTime endDate
-Set~TagTimeCategory~ selectedCategories
+Widget build(context)
}
class SectionHeader {
+String title
+IconData icon
+Widget trailing
+VoidCallback onTap
+TextStyle titleStyle
+Widget build(context)
}
ThemeDataBuilder --> PagePaddingTheme : builds
AppTheme --> ThemeDataBuilder : used by
ThemeDataBuilder --> AppTheme : uses sizeSmall
BuildContext <|.. PagePaddingContext : extension
PagePaddingContext --> PagePaddingTheme : reads extension
PagePaddingContext --> AppTheme : uses screenSmall, sizeSmall
TagDetailsPage --> _TagDetailsPageState : creates state
_TagDetailsPageState --> TagStatisticsView : composes
_TagDetailsPageState --> BuildContext : uses pageBodyPadding
TagStatisticsView --> _TagStatisticsViewState : creates state
_TagStatisticsViewState --> TagTimeBarChart : composes
_TagStatisticsViewState --> TagTimeChartOptions : composes
_TagStatisticsViewState --> ITranslationService : uses
_TagStatisticsViewState --> IThemeService : uses
SectionHeader --> AppTheme : uses spacing and text styles
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've left some high level feedback:
- In
AppUsageList, switching fromListView.separatedtoListView.builderremoves the inter-item spacing that was previously provided byseparatorBuilder; if the tighter layout is not intentional, consider adding equivalent vertical spacing via item padding or explicitSizedBoxwidgets. - The new pattern of a left-aligned section label followed by horizontally scrollable/filter controls (e.g., in
AppUsageStatisticsView,TagsPage, andTagStatisticsView) is implemented in several places with nearly identicalPadding/Rowstructures; consider extracting a small reusable widget to avoid duplication and keep these headers visually consistent. - In
TagStatisticsView,_themeServiceis only used to obtain a primary color for the icon background; you can simplify this by dropping the injectedIThemeServiceand usingTheme.of(context).colorScheme.primaryinstead, which reduces coupling to the theme service.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `AppUsageList`, switching from `ListView.separated` to `ListView.builder` removes the inter-item spacing that was previously provided by `separatorBuilder`; if the tighter layout is not intentional, consider adding equivalent vertical spacing via item padding or explicit `SizedBox` widgets.
- The new pattern of a left-aligned section label followed by horizontally scrollable/filter controls (e.g., in `AppUsageStatisticsView`, `TagsPage`, and `TagStatisticsView`) is implemented in several places with nearly identical `Padding`/`Row` structures; consider extracting a small reusable widget to avoid duplication and keep these headers visually consistent.
- In `TagStatisticsView`, `_themeService` is only used to obtain a primary color for the icon background; you can simplify this by dropping the injected `IThemeService` and using `Theme.of(context).colorScheme.primary` instead, which reduces coupling to the theme service.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new PagePaddingTheme extension for consistent page-level padding across the application, automatically adjusting horizontal padding for small screens. This change involved updating numerous page and component files (e.g., AboutPage, AppUsageDetailsPage, HabitDetailsPage, NoteDetailsPage, PermissionsPage, MarathonPage, TaskDetailsPage, ResponsiveScaffoldLayout, AddSyncDevicePage, SyncDevicesPage) to utilize context.pageBodyPadding instead of hardcoded EdgeInsets.all or EdgeInsets.symmetric values. Consequently, the SectionHeader widget was refactored to remove its padding property, as padding is now managed by the parent context. Additionally, the AppUsageStatisticsView was updated to embed its header and filter options directly, replacing a StatisticsSummaryCards widget with a custom _buildStatCard implementation for displaying usage statistics. The AppUsageList was changed from ListView.separated to ListView.builder, removing explicit separators. A new TagStatisticsView component was introduced to TagDetailsPage to display tag-specific time statistics, replacing a tabbed interface that previously included tasks and notes. The TagTimeBarChart was updated to use shrinkWrap and NeverScrollableScrollPhysics. A Container wrapping ListTile in TagCard was deemed redundant and could be removed, and a custom lerpDouble implementation in PagePaddingTheme was noted as duplicative of dart:ui's version. Finally, a demo mode check was added to AppUsageViewPage to bypass permission checks, and an import for acore.dart was modified to hide Container to resolve a naming conflict.
src/lib/presentation/ui/shared/services/theme_service/page_padding_theme.dart
Outdated
Show resolved
Hide resolved
Code reviewFound 1 issue:
whph/src/lib/presentation/ui/features/app_usages/components/app_usage_statistics_view.dart Lines 321 to 328 in 8da571f
The same issue occurs at: whph/src/lib/presentation/ui/features/app_usages/components/app_usage_statistics_view.dart Lines 328 to 335 in 8da571f
Both instances of |
🚀 Motivation and Context
Simplify complex UI structures by removing tab-based interfaces in favor of scrollable layouts. Introduce theme extension system for consistent page-level padding across the application. Consolidate inline component implementations to reduce architectural complexity while maintaining visual consistency.
⚙️ Implementation Details
This commit introduces a centralized theme-based spacing system and streamlines UI component layouts:
Theme Extension System:
PagePaddingThemeextension toThemeDatafor consistent page-level paddingtheme_servicewith centralized padding configurationapp_theme.dartwith new theme extension supportUI Layout Improvements:
tag_details_page.dart(353 → reduced lines) by extracting componentstoday_page.dartfor cleaner layout structuresettings_page.dartand component layoutsComponent Consolidation:
tag_statistics_view.dartas new shared componentresponsive_scaffold_layout.dartfor better consistencysection_header.dartspacingAffected Areas:
📋 Checklist for Reviewer
🔗 Related
No related issues or PRs.
Summary by Sourcery
Introduce a centralized theme-based page padding system and streamline multiple feature pages to use consistent scrollable layouts and simplified section headers.
New Features:
Enhancements: