-
Notifications
You must be signed in to change notification settings - Fork 31
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
Implemented the guest user experience and pages #2394
Conversation
📝 WalkthroughWalkthroughThis pull request introduces guest user functionality to the mobile application, enhancing user authentication and profile management. The changes span multiple files, implementing a new guest user flow that includes modifications to the authentication bloc, state management, and user interface components. The implementation allows users to use the app as a guest, with dedicated navigation, a profile page, and settings specifically designed for non-registered users. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
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.
Actionable comments posted: 8
🧹 Nitpick comments (13)
mobile-v3/lib/src/app/auth/bloc/auth_state.dart (1)
28-28
: Well-structured state implementation.Consider adding a brief documentation comment to describe the purpose of the
GuestUser
state for better code maintainability.+/// Represents the state when a user is accessing the app as a guest. final class GuestUser extends AuthState {}
mobile-v3/lib/src/app/auth/bloc/auth_bloc.dart (1)
44-45
: Consider adding error handling for guest mode.Unlike login/register flows, the guest mode transition lacks error handling. While it's a simpler flow, we should still handle potential failures in guest mode setup.
} else if (event is UseAsGuest) { - emit(GuestUser()); + try { + emit(AuthLoading()); + // Add any guest mode setup logic here + emit(GuestUser()); + } catch (e) { + debugPrint(e.toString()); + emit(AuthLoadingError(e.toString())); + } }mobile-v3/lib/src/app/profile/pages/widgets/guest_settings_widget.dart (2)
14-14
: Add error handling for asset loading.Asset paths are hardcoded without error handling. Consider implementing fallback icons and error handling for missing assets.
+// Create a common widget that handles asset loading with fallback +class SafeAssetIcon extends StatelessWidget { + final String assetPath; + const SafeAssetIcon(this.assetPath); + + @override + Widget build(BuildContext context) { + return SvgPicture.asset( + assetPath, + errorBuilder: (context, error, stackTrace) { + debugPrint('Failed to load asset: $assetPath'); + return Icon(Icons.error_outline); + }, + ); + } +}Also applies to: 23-23, 31-31, 38-38, 46-46
4-56
: Implement actual settings functionality.The settings widget currently only displays UI elements without actual functionality. Consider implementing the actual settings logic or marking these as TODO items in the code.
Would you like me to help create implementations for these settings features? I can provide examples for:
- Location permission handling
- Notification setup
- Feedback system integration
- Terms and privacy policy display
mobile-v3/lib/src/app/profile/pages/widgets/settings_tile.dart (1)
64-64
: Fix spacing in Divider color assignment.There's an inconsistent spacing before the colon in the color property.
- color:Theme.of(context).highlightColor, + color: Theme.of(context).highlightColor,mobile-v3/lib/src/meta/utils/colors.dart (1)
17-19
: Consider using semantic color names.While the current naming scheme works, consider using semantic names that describe the purpose rather than the variant number (e.g.,
headlineColorPrimary
,headlineColorSecondary
). This makes it easier to understand where each color should be used.mobile-v3/lib/src/app/auth/pages/welcome_screen.dart (1)
154-158
: Add semantic label to SVG icon for accessibility.The SVG icon lacks a semantic label, which impacts screen reader accessibility.
SvgPicture.asset('assets/icons/chevron-right.svg', height: 16.0, width: 16.0, -color: AppColors.boldHeadlineColor2,) +color: AppColors.boldHeadlineColor2, +semanticsLabel: 'Continue as guest arrow',)mobile-v3/lib/src/app/dashboard/pages/dashboard_page.dart (2)
105-122
: Improve profile navigation state handling.The current implementation directly reads the AuthBloc state without proper state handling for loading or error cases.
Consider using BlocBuilder for better state management:
- onTap: () { - final authState = context.read<AuthBloc>().state; - if (authState is GuestUser) { - Navigator.of(context).push( - MaterialPageRoute( - builder: (context) => GuestProfilePage(), - ), - ); - } else { - Navigator.of(context).push( - MaterialPageRoute( - builder: (context) => ProfilePage(), - ), - ); - } - }, + onTap: () => context.read<AuthBloc>().state.maybeWhen( + guest: () => Navigator.of(context).push( + MaterialPageRoute(builder: (context) => GuestProfilePage()), + ), + authenticated: () => Navigator.of(context).push( + MaterialPageRoute(builder: (context) => ProfilePage()), + ), + loading: () => null, // Handle loading state + error: (message) => ScaffoldMessenger.of(context).showSnackBar( + SnackBar(content: Text(message)), + ), + ),
183-217
: Reduce code duplication in greeting text styles.The greeting text implementation contains duplicate style definitions that could be extracted into a shared style.
Consider extracting the shared style:
+ final greetingStyle = TextStyle( + fontSize: 28, + fontWeight: FontWeight.w700, + color: AppColors.boldHeadlineColor2, + ); + BlocBuilder<AuthBloc, AuthState>( builder: (context, authState) { if (authState is GuestUser) { return Text( "Hi, Guest 👋🏼", - style: TextStyle( - fontSize: 28, - fontWeight: FontWeight.w700, - color: AppColors.boldHeadlineColor2, - ), + style: greetingStyle, ); } else { // ... rest of the code using greetingStyle } }, );mobile-v3/lib/src/app/profile/pages/guest_profile page.dart (4)
24-32
: Consider using a more intuitive navigation pattern.The close icon (X) might not be immediately intuitive for navigation. Consider:
- Using a back arrow which is more common in mobile apps
- Adding a title to the AppBar for better context
appBar: AppBar( automaticallyImplyLeading: false, + title: Text('Profile'), actions: [ IconButton( onPressed: () => Navigator.pop(context), - icon: Icon(Icons.close)), + icon: Icon(Icons.arrow_back)), - SizedBox(width: 16) ], ),
44-65
: Internationalize text and standardize margins.A few suggestions for improvement:
- The "Guest User" text should be internationalized for multi-language support
- Consider extracting margin values to constants for consistency
Container( - margin: const EdgeInsets.symmetric(horizontal: 20), + margin: const EdgeInsets.symmetric(horizontal: kDefaultMargin), child: CircleAvatar( backgroundColor: Theme.of(context).highlightColor, child: Center( child: SvgPicture.asset( "assets/icons/user_icon.svg"), ), radius: 50, ), ), Text( - "Guest User", + AppLocalizations.of(context).guestUser, style: TextStyle( color: AppColors.boldHeadlineColor2, fontSize: 24, fontWeight: FontWeight.w700, ), ),
88-91
: Clean up unnecessary empty lines.There are multiple consecutive empty lines that should be removed for better code organization.
124-129
: Optimize layout and extract constants.The current implementation has several areas for improvement:
- The bottom padding (220) seems excessive and should be reviewed
- Asset paths should be centralized in a constants file
- Consider using a more flexible approach for bottom spacing, such as SafeArea
+const String kSharedFrameAsset = 'assets/images/shared/Frame 26085560.svg'; Center( - child: SvgPicture.asset('assets/images/shared/Frame 26085560.svg') + child: SvgPicture.asset(kSharedFrameAsset) ), -SizedBox( - height: 220, -), +SafeArea( + minimum: EdgeInsets.only(bottom: 16), + child: SizedBox(), +),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
mobile-v3/assets/icons/chevron-right.svg
is excluded by!**/*.svg
mobile-v3/assets/images/shared/Frame 26085560.svg
is excluded by!**/*.svg
📒 Files selected for processing (11)
mobile-v3/lib/main.dart
(2 hunks)mobile-v3/lib/src/app/auth/bloc/auth_bloc.dart
(1 hunks)mobile-v3/lib/src/app/auth/bloc/auth_event.dart
(1 hunks)mobile-v3/lib/src/app/auth/bloc/auth_state.dart
(1 hunks)mobile-v3/lib/src/app/auth/pages/welcome_screen.dart
(2 hunks)mobile-v3/lib/src/app/dashboard/pages/dashboard_page.dart
(6 hunks)mobile-v3/lib/src/app/dashboard/widgets/analytics_card.dart
(2 hunks)mobile-v3/lib/src/app/profile/pages/guest_profile page.dart
(1 hunks)mobile-v3/lib/src/app/profile/pages/widgets/guest_settings_widget.dart
(1 hunks)mobile-v3/lib/src/app/profile/pages/widgets/settings_tile.dart
(2 hunks)mobile-v3/lib/src/meta/utils/colors.dart
(1 hunks)
🔇 Additional comments (4)
mobile-v3/lib/src/app/auth/bloc/auth_event.dart (1)
23-25
: Clean implementation of the guest user event!The
UseAsGuest
event follows Dart best practices with proper use of const constructor and sealed class hierarchy.mobile-v3/lib/src/app/profile/pages/widgets/settings_tile.dart (1)
41-43
: LGTM! Typography improvements enhance readability.The updated font sizes and weights create better visual hierarchy and improve readability.
Also applies to: 60-61
mobile-v3/lib/src/app/dashboard/widgets/analytics_card.dart (1)
61-64
: LGTM! Consistent typography and color updates.The styling changes improve visual hierarchy and maintain consistency with the new color system.
Also applies to: 70-70, 102-102, 107-107
mobile-v3/lib/src/app/auth/pages/welcome_screen.dart (1)
46-54
: LGTM! Clean implementation of guest user navigation.The BlocListener implementation with Future.microtask for navigation is a solid approach to avoid build-phase navigation issues.
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.
Thanks @Karagwa for the PR! Good stuff! :)
I await review notes from @Mozart299 and I will proceed with merge duty accordingly.
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.
Actionable comments posted: 0
♻️ Duplicate comments (1)
mobile-v3/lib/src/app/profile/pages/guest_profile page.dart (1)
115-136
: 🛠️ Refactor suggestionImprove button implementation for better accessibility and maintainability.
The current button implementation using
InkWell
+Container
should be replaced withElevatedButton
for better accessibility.- InkWell( - onTap: () => Navigator.of(context).push(MaterialPageRoute( - builder: (context) => CreateAccountScreen())), - child: Container( - height: 56, - decoration: BoxDecoration( - color: AppColors.primaryColor, - borderRadius: - BorderRadius.circular(4)), - child: Center( - child: isLoading - ? Spinner() - : Text( - "Create Account", - style: TextStyle( - fontWeight: FontWeight.w500, - color: Colors.white, - ), - ), - ), - ), - ), + ElevatedButton( + onPressed: isLoading ? null : navigateToCreateAccount, + style: ElevatedButton.styleFrom( + minimumSize: const Size(double.infinity, 56), + shape: RoundedRectangleBorder( + borderRadius: BorderRadius.circular(4), + ), + ), + child: isLoading + ? const Spinner() + : const Text( + "Create Account", + style: TextStyle( + fontWeight: FontWeight.w500, + ), + ), + ),
🧹 Nitpick comments (9)
mobile-v3/lib/src/app/profile/pages/guest_profile page.dart (5)
1-2
: Remove or uncomment the commented import.The commented import
settings_tile.dart
should either be removed if unused or uncommented if needed. Keeping commented imports makes the codebase unclear and harder to maintain.-//import 'package:airqo/src/app/profile/pages/widgets/settings_tile.dart';
27-29
: Remove simulated delay in production code.The simulated delay should be removed from production code. If this is for testing purposes, consider moving it behind a debug flag.
- // Simulate a delay for account creation or call an API here - await Future.delayed(Duration(seconds: 2));
22-36
: Consolidate navigation logic.The
handleCreateAccount
method and the button'sonTap
handler both navigate toCreateAccountScreen
, creating duplicate logic. Consider using a single navigation method.- void handleCreateAccount() async { - setState(() { - isLoading = true; - }); - - await Future.delayed(Duration(seconds: 2)); - - setState(() { - isLoading = false; - }); - - Navigator.of(context).push(MaterialPageRoute(builder: (context) => CreateAccountScreen())); - } + Future<void> navigateToCreateAccount() async { + setState(() { + isLoading = true; + }); + + try { + await Navigator.of(context).push( + MaterialPageRoute(builder: (context) => CreateAccountScreen()) + ); + } finally { + setState(() { + isLoading = false; + }); + } + }
74-81
: Extract text styles to theme or constants.Hardcoded text styles should be moved to the theme or a dedicated styles file for consistency and maintainability.
Consider creating a styles file:
class TextStyles { static const guestUserTitle = TextStyle( fontSize: 24, fontWeight: FontWeight.w700, ); static const sectionHeader = TextStyle( fontSize: 18, fontWeight: FontWeight.w500, ); }Also applies to: 87-93
50-149
: Extract widget sections into separate components.The build method is quite large with nested widgets. Consider extracting logical sections into separate widget components for better maintainability and reusability.
Suggested structure:
_GuestProfileHeader
(avatar and name)_SettingsSection
(settings header and list)_CreateAccountButton
_FooterSection
(bottom image and spacing)mobile-v3/lib/src/app/auth/pages/welcome_screen.dart (1)
142-156
: Consider adding loading state feedback.The guest mode button could benefit from visual feedback while the UseAsGuest event is being processed. This would improve the user experience by indicating that their action is being handled.
InkWell( - onTap: () => context.read<AuthBloc>().add(UseAsGuest()), + onTap: () { + context.read<AuthBloc>().add(UseAsGuest()); + // Show loading indicator + ScaffoldMessenger.of(context).showSnackBar( + SnackBar( + content: Row( + children: [ + SizedBox( + height: 20, + width: 20, + child: CircularProgressIndicator(strokeWidth: 2), + ), + SizedBox(width: 16), + Text('Entering guest mode...'), + ], + ), + duration: Duration(seconds: 1), + ), + ); + },mobile-v3/lib/src/app/dashboard/pages/dashboard_page.dart (3)
109-126
: Consider handling loading state during navigation.While the navigation logic is solid, consider adding a loading state while checking the auth state to prevent potential UI flickers.
onTap: () { + showDialog( + context: context, + barrierDismissible: false, + builder: (context) => Center(child: CircularProgressIndicator()), + ); final authState = context.read<AuthBloc>().state; if (authState is GuestUser) { + Navigator.pop(context); // Dismiss loading Navigator.of(context).push( MaterialPageRoute( builder: (context) => GuestProfilePage(), ), ); } else { + Navigator.pop(context); // Dismiss loading Navigator.of(context).push( MaterialPageRoute( builder: (context) => ProfilePage(), ), ); } },
127-171
: Consider extracting avatar widget to reduce nesting.The nested BlocBuilder implementation makes the code harder to maintain. Consider extracting the avatar logic into a separate widget.
+class UserAvatar extends StatelessWidget { + @override + Widget build(BuildContext context) { + return BlocBuilder<AuthBloc, AuthState>( + builder: (context, authState) { + if (authState is GuestUser) { + return _buildGuestAvatar(context); + } + return _buildUserAvatar(context); + }, + ); + } + + Widget _buildGuestAvatar(BuildContext context) { + return CircleAvatar( + backgroundColor: Theme.of(context).highlightColor, + child: Center( + child: SvgPicture.asset( + "assets/icons/user_icon.svg", + height: 22, + width: 22, + ), + ), + radius: 24, + ); + } + + Widget _buildUserAvatar(BuildContext context) { + return BlocBuilder<UserBloc, UserState>( + builder: (context, userState) => _buildAvatarFromUserState(context, userState), + ); + } +}
187-224
: Consider extracting greeting widget for better maintainability.The greeting implementation follows a similar pattern to the avatar. Consider extracting it into a separate widget to improve code organization and reusability.
+class UserGreeting extends StatelessWidget { + @override + Widget build(BuildContext context) { + return BlocBuilder<AuthBloc, AuthState>( + builder: (context, authState) { + if (authState is GuestUser) { + return _buildGuestGreeting(); + } + return _buildUserGreeting(context); + }, + ); + } + + Widget _buildGuestGreeting() { + return Text( + "Hi, Guest 👋🏼", + style: TextStyle( + fontSize: 28, + fontWeight: FontWeight.w700, + color: AppColors.boldHeadlineColor2, + ), + ); + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
mobile-v3/lib/main.dart
(1 hunks)mobile-v3/lib/src/app/auth/pages/welcome_screen.dart
(2 hunks)mobile-v3/lib/src/app/dashboard/pages/dashboard_page.dart
(6 hunks)mobile-v3/lib/src/app/profile/pages/guest_profile page.dart
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mobile-v3/lib/main.dart
🔇 Additional comments (4)
mobile-v3/lib/src/app/auth/pages/welcome_screen.dart (3)
46-54
: Consider unifying the guest navigation logic.The BlocListener's navigation logic using
Future.microtask
could potentially race with the direct navigation from the "Continue as guest" button. Consider handling all navigation through the BlocListener.
54-90
: LGTM! Well-structured layout implementation.Good use of MediaQuery for responsive design and proper implementation of the PageView for onboarding screens.
101-139
: LGTM! Well-implemented authentication buttons.Good use of theming for dark mode support and proper navigation implementation.
mobile-v3/lib/src/app/dashboard/pages/dashboard_page.dart (1)
45-50
: Reconsider guest mode initialization logic.The current implementation might force guest mode even when it's not intended. Consider checking for existing authentication before setting guest mode.
Summary of Changes (What does this PR do?)
Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Screenshots (optional)
Summary by CodeRabbit
Release Notes
New Features
UI/UX Changes
Theme Updates
The release introduces a more flexible authentication experience, allowing users to explore the app as guests with a dedicated profile and settings interface.