Merge feature/improve-login-page-with-tests-tur-blackbox into master#8
Conversation
Summary of ChangesHello @busratuter, 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 enhances the user authentication and registration experience by introducing a new, dedicated login screen and thoroughly refactoring the existing registration form. It centralizes input validation logic into a reusable utility and improves custom form widgets for better state management and user interaction. The changes are backed by a comprehensive suite of unit tests, ensuring the robustness and correctness of the updated components. Highlights
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the application by refactoring the registration page into a stateful widget, adding a new login page, implementing form validation, and introducing a comprehensive suite of tests. The use of named routes and modern Flutter practices like const constructors and updated buttons is excellent. My review focuses on improving null safety, enhancing widget reusability, fixing a few minor bugs in both the application and test code, and addressing some maintainability and accessibility concerns. Overall, this is a high-quality contribution.
| } | ||
|
|
||
| void _handleLogin() { | ||
| if (_formKey.currentState!.validate()) { |
There was a problem hiding this comment.
Using the bang operator ! on _formKey.currentState can lead to a runtime crash if the form state is unexpectedly null. It's safer to use a null-aware access ?. with a null check to prevent this.
| if (_formKey.currentState!.validate()) { | |
| if (_formKey.currentState?.validate() ?? false) { |
| } | ||
|
|
||
| void _handleSave() { | ||
| if (_formKey.currentState!.validate()) { |
There was a problem hiding this comment.
Using the bang operator ! on _formKey.currentState can lead to a runtime crash if the form state is unexpectedly null. It's safer to use a null-aware access ?. with a null check to prevent this.
| if (_formKey.currentState!.validate()) { | |
| if (_formKey.currentState?.validate() ?? false) { |
| ), | ||
| child: Form( | ||
| key: _formKey, | ||
| child: Column( |
There was a problem hiding this comment.
This registration form is missing fields for password creation and confirmation. A user registration process should allow the user to set a password. Consider adding password and confirm password fields. You could use the PasswordField widget that has been created in lib/widgets/password_field.dart but is currently unused.
| final emailRegex = RegExp( | ||
| r'^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$', | ||
| ); | ||
|
|
||
| if (!emailRegex.hasMatch(value)) { | ||
| return 'Please enter a valid email'; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| static String? validatePassword(String? value) { | ||
| if (value == null || value.isEmpty) { | ||
| return 'Password is required'; | ||
| } | ||
|
|
||
| if (value.length < 6) { | ||
| return 'Password must be at least 6 characters'; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| static String? validatePhone(String? value) { | ||
| if (value == null || value.isEmpty) { | ||
| return 'Phone number is required'; | ||
| } | ||
|
|
||
| final phoneRegex = RegExp(r'^\+?[0-9]{10,15}$'); | ||
|
|
||
| if (!phoneRegex.hasMatch(value.replaceAll(RegExp(r'\s+'), ''))) { |
There was a problem hiding this comment.
Creating a RegExp instance inside a function means it will be recompiled on every call. For better performance, especially in a validator that might be called frequently, define this RegExp as a top-level final variable outside the class so it's compiled only once.
| import 'package:login_page/widgets/input_field.dart'; | ||
| import 'package:login_page/widgets/password_field.dart'; |
There was a problem hiding this comment.
These imports for input_field.dart and password_field.dart are not used in this file. They should be removed to keep the code clean and reduce clutter.
| constraints: BoxConstraints( | ||
| maxWidth: MediaQuery.of(context).size.width / 3.7, | ||
| ), |
There was a problem hiding this comment.
Hardcoding the maxWidth with MediaQuery.of(context).size.width / 3.7 makes this InputField widget less reusable, as it's tightly coupled to the layout of the screen where it's currently used. It's generally better to let the parent widget control the size and layout. The Expanded widget is already handling the space. Consider removing these constraints to make the widget more flexible.
| borderSide: BorderSide(color: Colors.blue[50]!), | ||
| borderRadius: BorderRadius.circular(5.0), | ||
| ), | ||
| decoration: InputDecoration( | ||
| contentPadding: EdgeInsets.all(10.0), | ||
| border: OutlineInputBorder( | ||
| borderSide: BorderSide( | ||
| color: Colors.blue[50], | ||
| ), | ||
| borderRadius: BorderRadius.circular(5.0), | ||
| ), | ||
| enabledBorder: OutlineInputBorder( | ||
| borderSide: BorderSide( | ||
| color: Colors.blue[50], | ||
| ), | ||
| borderRadius: BorderRadius.circular(5.0), | ||
| ), | ||
| hintText: "$content", | ||
| fillColor: Colors.blue[50], | ||
| enabledBorder: OutlineInputBorder( | ||
| borderSide: BorderSide(color: Colors.blue[50]!), |
There was a problem hiding this comment.
Using the bang operator ! on Colors.blue[50] is risky. While it's unlikely to be null for a MaterialColor, it's safer to provide a fallback value to prevent potential crashes, for example Colors.blue[50] ?? Colors.grey.
| borderSide: BorderSide(color: Colors.blue[50]!), | |
| borderRadius: BorderRadius.circular(5.0), | |
| ), | |
| decoration: InputDecoration( | |
| contentPadding: EdgeInsets.all(10.0), | |
| border: OutlineInputBorder( | |
| borderSide: BorderSide( | |
| color: Colors.blue[50], | |
| ), | |
| borderRadius: BorderRadius.circular(5.0), | |
| ), | |
| enabledBorder: OutlineInputBorder( | |
| borderSide: BorderSide( | |
| color: Colors.blue[50], | |
| ), | |
| borderRadius: BorderRadius.circular(5.0), | |
| ), | |
| hintText: "$content", | |
| fillColor: Colors.blue[50], | |
| enabledBorder: OutlineInputBorder( | |
| borderSide: BorderSide(color: Colors.blue[50]!), | |
| borderSide: BorderSide(color: Colors.blue[50] ?? Colors.grey), | |
| borderRadius: BorderRadius.circular(5.0), | |
| ), | |
| enabledBorder: OutlineInputBorder( | |
| borderSide: BorderSide(color: Colors.blue[50] ?? Colors.grey), |
| Widget _buildMembershipOption(String type, String label) { | ||
| final isSelected = _selectedMembership == type; | ||
| return Row( | ||
| mainAxisSize: MainAxisSize.min, | ||
| children: [ | ||
| InkWell( | ||
| onTap: () { | ||
| setState(() { | ||
| _selectedMembership = type; | ||
| }); | ||
| widget.onChanged?.call(type); | ||
| }, | ||
| child: CircleAvatar( | ||
| backgroundColor: isSelected ? Colors.blue[300] : Colors.blue[50], | ||
| child: Icon( | ||
| Icons.account_balance_wallet, | ||
| color: isSelected ? Colors.white : Colors.grey, | ||
| ), | ||
| ], | ||
| ); | ||
| }, | ||
| ), | ||
| ), | ||
| const SizedBox(width: 10.0), | ||
| Container( | ||
| width: 50.0, | ||
| child: Text( | ||
| label, | ||
| textAlign: TextAlign.left, | ||
| ), | ||
| ), | ||
| ], | ||
| ); | ||
| } |
There was a problem hiding this comment.
Similar to the Gender widget, the InkWell only wraps the CircleAvatar. For better usability, the tappable area should include the text label as well. Consider wrapping the entire Row returned by _buildMembershipOption in the InkWell.
| constraints: BoxConstraints( | ||
| maxWidth: MediaQuery.of(context).size.width / 3.7, | ||
| ), | ||
| child: TextFormField( | ||
| controller: widget.controller, | ||
| obscureText: _obscureText, | ||
| validator: widget.validator, | ||
| style: const TextStyle(fontSize: 15.0), | ||
| decoration: InputDecoration( | ||
| contentPadding: const EdgeInsets.all(10.0), | ||
| border: OutlineInputBorder( | ||
| borderSide: BorderSide(color: Colors.blue[50]!), | ||
| borderRadius: BorderRadius.circular(5.0), | ||
| ), | ||
| enabledBorder: OutlineInputBorder( | ||
| borderSide: BorderSide(color: Colors.blue[50]!), | ||
| borderRadius: BorderRadius.circular(5.0), | ||
| ), | ||
| filled: true, | ||
| fillColor: Colors.blue[50], | ||
| suffixIcon: IconButton( | ||
| icon: Icon( | ||
| _obscureText ? Icons.visibility : Icons.visibility_off, | ||
| color: Colors.grey, | ||
| ), | ||
| onPressed: () { | ||
| setState(() { | ||
| _obscureText = !_obscureText; | ||
| }); | ||
| }, | ||
| ), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
This widget has a couple of areas for improvement:
- Reusability: The hardcoded
maxWidthconstraint (MediaQuery.of(context).size.width / 3.7) makes the widget less reusable. It's better to let the parent widget control the width. - Robustness: Using the
!operator onColors.blue[50]is risky. Provide a fallback color to be safe.
Also, note that this widget is currently not used anywhere in the project. It should either be integrated (e.g., into the registration form) or removed.
|
|
||
| // Find email field and enter invalid email | ||
| final emailFields = find.byType(TextFormField); | ||
| await tester.enterText(emailFields.at(3), 'invalid-email'); |
There was a problem hiding this comment.
This test for invalid email format seems to be targeting the wrong field. emailFields.at(3) corresponds to the 'Mobile' field, not the 'Email' field, which is at index 2. This will cause the test to pass incorrectly or fail for the wrong reason.
| await tester.enterText(emailFields.at(3), 'invalid-email'); | |
| await tester.enterText(emailFields.at(2), 'invalid-email'); |
2 participants
Automated PR created from task completion
This PR was created from task: https://cloud.blackbox.ai/tasks/Wax1OcnLmL5p8zqhi4F8j