Menu tab #360
Menu tab #360
Conversation
|
Seems the analyzer is complaining. In VSCode, it's a good idea to enable the "Format file on Save" option, such that the dart formatter will always make sure your files are formatted properly. |
lib/main.dart
Outdated
| @@ -1,4 +1,4 @@ | |||
| import 'dart:async'; | |||
| import 'dart:async'; | |||
| Widget _buildSubTitle( | ||
| {required String subTitle, | ||
| double paddingTop = 0.0, | ||
| double paddingBottom = 0.0}) { |
| class MenuPage extends StatefulWidget { | ||
| const MenuPage({Key? key}) : super(key: key); | ||
|
|
||
| @override | ||
| State<MenuPage> createState() => _MenuTabState(); | ||
| } | ||
|
|
||
| class _MenuTabState extends State<MenuPage> { | ||
| @override | ||
| Widget build(BuildContext context) { |
There was a problem hiding this comment.
No reason for this being a Statefulwidget, can be stateless.
| Row( | ||
| mainAxisAlignment: MainAxisAlignment.spaceBetween, | ||
| crossAxisAlignment: CrossAxisAlignment.center, | ||
| children: [ | ||
| Text( | ||
| "Menu", | ||
| style: TextStyle( | ||
| fontSize: 28, | ||
| fontFamily: "Rubik", | ||
| fontWeight: FontWeight.w700, | ||
| ), | ||
| ), | ||
| IconButton( | ||
| onPressed: () {}, | ||
| icon: Icon( | ||
| Icons.search, | ||
| color: kPrimaryColor600, | ||
| size: 21.08, | ||
| ), | ||
| ), | ||
| ], | ||
| ), |
There was a problem hiding this comment.
This should most likely be a common widget. Best to extract it.
| "Menu", | ||
| style: TextStyle( | ||
| fontSize: 28, | ||
| fontFamily: "Rubik", |
| ), | ||
| decoration: BoxDecoration( | ||
| color: Colors.white, | ||
| shape: BoxShape.circle), |
| SizedBox( | ||
| height: 30, | ||
| ), | ||
| Container( |
There was a problem hiding this comment.
This should be a Widget itself as well.
| ), | ||
| ), | ||
| _buildSubTitle( | ||
| subTitle: "Account", paddingTop: 38.5, paddingBottom: 20), |
| class _AvatarAndInfoState extends State<AvatarAndInfo> { | ||
| late final IUserRepository _userRepository; | ||
| File? _image; | ||
| @override | ||
| void initState() { | ||
| super.initState(); | ||
| _userRepository = getIt<IUserRepository>(); | ||
| } | ||
| @override | ||
| Widget build(BuildContext context) { |
There was a problem hiding this comment.
Would love to see some line breaks..
| // ProfilePicture( | ||
| // maxRadius: 40, | ||
| // profileImage: | ||
| // '${dotenv.get('BASE_STATIC_ENDPOINT_URL')}/${widget.pictureUrl}', | ||
| // ), |
| profileImage: widget.pictureUrl != | ||
| null | ||
| ? '${dotenv.get('BASE_STATIC_ENDPOINT_URL')}/${widget.pictureUrl}' | ||
| : null, |
There was a problem hiding this comment.
Null check but if null then you return null? Look into ?? operator.
| },stream: _userRepository.observeUser() | ||
| ,), |
| } else { | ||
| return const Text('...'); | ||
| } |
There was a problem hiding this comment.
No need for the else, just do:
if (snapshot.hasData) {
return Text(snapshot.data!.phoneNumber!, style: phoneNumberTextStyle);
}
return const Text('...', style: phoneNumberTextStyle);
| @@ -15,7 +15,7 @@ class BuildInformationTile extends StatelessWidget { | |||
| const SizedBox(height: 50), | |||
| const SizedBox( | |||
| width: 56, | |||
| child: Image(image: AssetImage('assets/images/build_info.png')), | |||
There was a problem hiding this comment.
If we're not using build_info.png anywhere else, we can delete it from the assets folder.
| {Key? key, | ||
| required this.onTap, | ||
| required this.iconWidget, | ||
| required this.label}) |
| import '../../themes/constants.dart'; | ||
|
|
||
| class LegalInfoAndPoliciesWidget extends StatefulWidget { | ||
| final onTap; |
| onTap:(){ | ||
| widget.onTap(); | ||
| }, |
There was a problem hiding this comment.
| onTap:(){ | |
| widget.onTap(); | |
| }, | |
| onTap:() => widget.onTap(), |
| const Color kPrimaryColor600 = Color(0xFF2EB494); | ||
| const TextStyle kCaption1 = TextStyle( |
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Too many line breaks, use the formatter
pubspec.yaml
Outdated
| @@ -39,6 +39,7 @@ dependencies: | |||
| flutter_bloc: ^8.1.1 | |||
| flutter_dotenv: ^5.0.2 | |||
| freezed_annotation: ^2.2.0 | |||
| fvm: ^2.4.1 | |||
There was a problem hiding this comment.
What is this? We don't use it nor need it?
pubspec.yaml
Outdated
| dependency_overrides: | ||
| firebase_core_platform_interface: 4.5.1 |
|
Formatter still fails |
It's specifically complaining about Tests are also failing |
| ); | ||
| } | ||
|
|
||
| Widget _buildSubTitle({ |
There was a problem hiding this comment.
Don't use methods returning Widgets if there is no reason to do so.
Make it into a StatelessWidget with a constant constructor. Functions and methods returning widgets that are used in Build methods are a pitfall for useless rebuilding.
| SizedBox( | ||
| width: 306, | ||
| child: Text( | ||
| "View and update your account and contact information that is associated with your CollAction account.", | ||
| style: TextStyle( | ||
| fontSize: 12, | ||
| color: kPrimaryColor300, | ||
| fontWeight: FontWeight.w400), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
Why are you using a fixed width here (why even a sizedbox?)? This won't make it look well on devices.
There was a problem hiding this comment.
I will fix it soon, sorry I got exams 😥.
…ncy, make readable
|
I've pushed a commit. There were a lot of discrepancies according to Design. Take a look at my commit and it's changes, and have the figma beside you when you look it through. Some of the generic mistakes:
And some other minor things. |
There was a problem hiding this comment.
@AbedrahmanYassen The design looks good, just add some fixes.
| padding: const EdgeInsets.only(left: 20, right: 54) + | ||
| const EdgeInsets.symmetric(vertical: 10), |
There was a problem hiding this comment.
Same for this. Use EdgeInsets.fromLTRB
| style: TextStyle( | ||
| fontSize: 14, | ||
| fontWeight: FontWeight.w500, | ||
| color: kPrimaryColor300, | ||
| ), |
There was a problem hiding this comment.
It's not easily visible but we could take into consideration the line height from figma just to make it pixel perfect.
Same for other text styles.
There was a problem hiding this comment.
how much line-height should I add ?
|
|
||
| import '../../themes/constants.dart'; | ||
|
|
||
| class HeaderBar extends StatelessWidget { |
There was a problem hiding this comment.
Retire this after implementation of AppBar instead.
There was a problem hiding this comment.
We have the implementation of the search bar, I think It's easier to implement the search bar, if It's like this not an AppBar, What do you think?
| children: [ | ||
| AvatarAndInfo( | ||
| avatar: avatar, | ||
| phoneNumber: '+31 612345678', |
There was a problem hiding this comment.
Add a TODO: To get the phone number from the profile.
There was a problem hiding this comment.
There is no need to get the phone number from the profile, once the user has signed in, the phone number will be showed.
| @@ -23,7 +24,7 @@ class ProfilePicture extends StatelessWidget { | |||
|
|
|||
| if (profileImage != null && imageProvider == null) { | |||
| imageProvider = NetworkImage( | |||
| profileImage!, | |||
| "${dotenv.env['BASE_STATIC_ENDPOINT_URL']}/$profileImage", | |||
There was a problem hiding this comment.
Create a config file with static url instead of directly using dotenv.
There was a problem hiding this comment.
This is also the reason the profile_picture_test.dart is failing
| @@ -19,8 +19,6 @@ class BuildInformationBloc | |||
| on<BuildInformationEvent>((event, emit) async { | |||
| await event.when( | |||
| fetch: () async { | |||
| emit(const BuildInformationState.loading()); | |||
There was a problem hiding this comment.
Any particular reason for removing the loading state ??
There was a problem hiding this comment.
It's also the reason the build_information_test and building_information_bloc_test are failing
There was a problem hiding this comment.
This one is tricky, it should be the initial state of the BLOC
There was a problem hiding this comment.
The blocTest expects only emitted values when an event is added. So skipping the initial state.
Co-authored-by: Isaac Obella <[email protected]>
Co-authored-by: Isaac Obella <[email protected]>
Codecov Report
@@ Coverage Diff @@
## development #360 +/- ##
===============================================
- Coverage 65.88% 56.49% -9.39%
===============================================
Files 161 157 -4
Lines 4095 4064 -31
===============================================
- Hits 2698 2296 -402
- Misses 1397 1768 +371
... and 6 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
I have made the menu tab and used the logic at the settings screen in this tab, after that I have deleted the settings screen.