UI Consistency Improvements - Issue #92 #121
Conversation
WalkthroughThe drawer implementation across several screens was refactored. The previous use of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AppBar
participant DrawerProvider
participant CustomDrawerOverlay
participant Navigator
User->>AppBar: Tap menu icon
AppBar->>DrawerProvider: toggleDrawer()
DrawerProvider-->>CustomDrawerOverlay: drawer state changes to open
CustomDrawerOverlay->>User: Show overlay drawer
User->>CustomDrawerOverlay: Tap menu item
CustomDrawerOverlay->>DrawerProvider: closeDrawer()
CustomDrawerOverlay->>Navigator: Navigate to selected route
DrawerProvider-->>CustomDrawerOverlay: drawer state changes to closed
CustomDrawerOverlay->>User: Hide overlay drawer
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 3
🔭 Outside diff range comments (2)
lib/features/home/screens/home_screen.dart (2)
99-101: Compile-time error:Color.withValuesSame API misuse as the app bar:
- color: Colors.white.withValues(alpha: 0.1), + color: Colors.white.withOpacity(0.1),
188-191: AllwithValuesusages need replacementThese two lines also need
withOpacityto compile:- border: Border.all(color: Colors.white.withValues(alpha: 0.05)), - ... - color: Colors.black.withValues(alpha: 0.2), + border: Border.all(color: Colors.white.withOpacity(0.05)), + ... + color: Colors.black.withOpacity(0.2),Search the repo for any remaining occurrences.
🧹 Nitpick comments (6)
lib/shared/providers/drawer_provider.dart (1)
3-9: Consider auto-disposing the notifier or adding documentation
DrawerNotifieris declared as a global, non-autoDisposeprovider.
If the drawer is only relevant while the app is in the foreground, you can save memory and prevent accidental state leaks by switching toautoDisposeor by adding a short comment clarifying why the state must outlive the UI.No action required if global persistence is intentional.
lib/shared/widgets/mostro_app_bar.dart (1)
35-36: Minor: inline notifier readInstead of
ref.read(drawerProvider.notifier).toggleDrawer();you can callref.read(drawerProvider.notifier).toggleDrawer()once and avoid repeated rebuilds by usinglistenorwatch, but that’s optional here.lib/features/home/screens/home_screen.dart (1)
24-88: Bottom navigation should be inScaffold.bottomNavigationBarEmbedding
BottomNavBarinside theCustomDrawerOverlaymeans it scrolls with content and is covered by the drawer animation.
Consider moving it back toScaffold.bottomNavigationBarfor a fixed, platform-consistent behaviour.lib/shared/widgets/custom_drawer_overlay.dart (2)
39-44: Avoid recomputing drawer width multiple times
MediaQuery.of(context).size.width * 0.7is evaluated twice (once forleft, once forwidth). Cache it in a localfinal drawerWidth = …;to avoid redundant work and guarantee both expressions stay in sync if the widget is rebuilt during an orientation change.-AnimatedPositioned( +final drawerWidth = MediaQuery.of(context).size.width * 0.7; +AnimatedPositioned( … - left: isDrawerOpen ? 0 : -MediaQuery.of(context).size.width * 0.7, + left: isDrawerOpen ? 0 : -drawerWidth, … - child: Container( - width: MediaQuery.of(context).size.width * 0.7, + child: Container( + width: drawerWidth,
129-132: Prevent duplicate navigation pushes
context.push(route)is executed unconditionally. If the user is already on the target route this will add an identical copy on the stack. Guard against this to keep the navigation stack clean:final currentLocation = GoRouter.of(context).location; if (currentLocation != route) { context.push(route); }lib/shared/widgets/mostro_app_drawer.dart (1)
10-17: RedundantMediaQuerycalculationsSame remark as in the new overlay: width is computed twice. Refactor with a local
drawerWidthvariable for consistency and readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/features/chat/screens/chat_rooms_list.dart(2 hunks)lib/features/home/screens/home_screen.dart(2 hunks)lib/features/trades/screens/trades_screen.dart(2 hunks)lib/shared/providers/drawer_provider.dart(1 hunks)lib/shared/widgets/custom_drawer_overlay.dart(1 hunks)lib/shared/widgets/mostro_app_bar.dart(3 hunks)lib/shared/widgets/mostro_app_drawer.dart(1 hunks)
🔇 Additional comments (4)
lib/features/trades/screens/trades_screen.dart (1)
25-98: Bottom nav placement mirrors the Home screen concern
BottomNavBaris placed inside the overlay; same UX issue as above (scrolling & z-index). Align both screens for consistency.lib/features/chat/screens/chat_rooms_list.dart (1)
25-48: Verify drawer z-index vs. list scrollingBecause
CustomDrawerOverlaywraps the whole body, confirm that keyboard interactions (e.g., search bars) and list scrolling don’t conflict with the overlay’sStackorder.lib/shared/widgets/custom_drawer_overlay.dart (2)
58-67: Verify asset registration
assets/images/logo.pngis hard-coded. A missingpubspec.yamlasset entry will crash at runtime (Unable to load asset). Please ensure the asset is declared:flutter: assets: - assets/images/logo.png
115-134: Nice separation of concernsEncapsulating menu-item construction in
_buildMenuItemkeeps the build method concise and makes future additions straightforward. Well done.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
@AndreaDiazCorreia está muy bueno, solo le haría un par de ajustes para mejorar la UX:
- Que el drawer se pueda cerrar con swipe hacia la izquierda, como ya funcionaba antes.
- Agregar una línea divisoria entre el logo de Mostro y los navigation items, y poner el logo con la misma distancia entre el borde superior y esa línea
|
@Catrya le hice los cambios que pediste y ademas agregue lo que me comentaste el otro dia de que se pueda pasar de la pestaña de buy y sell con gestos |
| route: '/walkthrough', | ||
| ), | ||
| ], | ||
| // Espacio después del logo |
There was a problem hiding this comment.
please don't add comments on spanish
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
@AndreaDiazCorreia please resolve the conflicts and rebase to merge
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/shared/widgets/bottom_nav_bar.dart (1)
73-125: Consider the impact of removing Material ripple effects.The change from
Material+InkWelltoGestureDetectorremoves the native Material Design ripple effect that provides visual feedback when users tap navigation items. While this achieves the UI consistency goals mentioned in the PR, consider the following trade-offs:
User Experience: Users expect visual feedback when tapping interactive elements. Without ripple effects, the tap interaction might feel less responsive.
Accessibility:
InkWellprovides better accessibility support out-of-the-box compared toGestureDetector.Platform Consistency: Material Design guidelines recommend using ripple effects for interactive elements.
If you want to maintain the current visual design while preserving some feedback, consider adding a subtle color change or scale animation:
return Expanded( child: GestureDetector( onTap: () => _onItemTapped(context, index), + child: AnimatedContainer( + duration: const Duration(milliseconds: 100), + color: Colors.transparent, child: SizedBox( height: double.infinity, child: Column( // ... existing content ), ), + ), ), );Alternatively, if you want to keep the Material ripple but with custom styling, you could use:
return Expanded( - child: GestureDetector( - onTap: () => _onItemTapped(context, index), + child: Material( + color: Colors.transparent, + child: InkWell( + onTap: () => _onItemTapped(context, index), + splashColor: AppTheme.activeColor.withOpacity(0.1), + highlightColor: Colors.transparent, child: SizedBox( height: double.infinity, child: Column( // ... existing content ), ), + ), ), );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/features/home/screens/home_screen.dart(2 hunks)lib/shared/widgets/bottom_nav_bar.dart(1 hunks)lib/shared/widgets/custom_drawer_overlay.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/features/home/screens/home_screen.dart
- lib/shared/widgets/custom_drawer_overlay.dart
merge changes
…en and bottom nav bar
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
Hola @AndreaDiazCorreia sigue sin la barra de navegacion del home
|
@Catrya lo probe en todos los tamaños posibles y me sale igual hice un cambio chico, prueba a ver |
3 participants
This PR enhances the visual consistency and polish of the Mostro mobile app's navigation elements, addressing UI improvements outlined in issue #92.
Changes Made
🔧 AppBar Improvements
Color Consistency: Updated AppBar background to AppTheme.backgroundDark to match the BUY/SELL tabs section
Visual Separation: Added subtle white border (1px, 10% opacity) at the bottom of AppBar to create clear separation from tabs
📱 Custom Drawer Enhancements
Icon Modernization: Replaced Material Icons with Lucide icons for better visual consistency:
Account: Icons.person_outline_rounded → LucideIcons.user
Settings: Icons.settings_rounded → LucideIcons.settings
About: Icons.info_rounded → LucideIcons.info
Walkthrough: Icons.menu_book_rounded → LucideIcons.bookOpen
Border Addition: Added subtle white border (1px, 10% opacity) on the right side of drawer for visual separation
🏗️ Drawer Architecture Benefits
Our current overlay-based drawer implementation follows mobile UX best practices.
Summary by CodeRabbit