For review PR #7
Conversation
| import 'package:path_provider/path_provider.dart'; | ||
| import 'package:surf_flutter_study_jam_2023/views/ticket_open_view.dart'; | ||
|
|
||
| void main() async { |
There was a problem hiding this comment.
General comment for the entire code:
- There are a lot of errors in the linter. You need to keep an eye on them.
- There are some commented lines of code that need to be removed.
| } | ||
|
|
||
| class _TicketListViewState extends State<TicketListView> { | ||
| void _showModalBottomSheet(BuildContext context) {} |
There was a problem hiding this comment.
The _showModalBottomSheet method is defined but not used. If you don't need it, you should remove it to avoid confusion.
| Ticket ticket = snapshot.data![index]; | ||
| return Card( | ||
| child: ListTile( | ||
| leading: Icon(Icons.file_present), |
There was a problem hiding this comment.
You can use the const keyword to create some of the widgets that have constant properties, such as AppBar and ElevatedButton also, to improve performance.
| @@ -0,0 +1,28 @@ | |||
| import 'dart:io'; | |||
There was a problem hiding this comment.
The file name should be in snake case.
| import '../services/fileDownloader.dart'; | ||
|
|
||
| class TicketController { | ||
| final downloader = FileDownloader(); |
There was a problem hiding this comment.
To better comply with the Dependency Inversion Principle (DIP), this dependency should be extracted as a parameter of the class.
| } | ||
|
|
||
| // ignore: use_build_context_synchronously | ||
| showModalBottomSheet( |
There was a problem hiding this comment.
-
Remove
BuildContextfrom here. -
Remove
showModalBottomSheet. -
Your controller should instead say "Hey, view, something has happened. Can you display the bottom sheet with that data?"
| @@ -0,0 +1,19 @@ | |||
| import 'observer.dart'; | |||
|
|
|||
| abstract class Observable { | |||
There was a problem hiding this comment.
There is already an implemented class in Flutter called ChangeNotifier (and Listenable contract).
|
|
||
| import '../services/observable.dart'; | ||
|
|
||
| class Ticket extends Observable { |
There was a problem hiding this comment.
This is a pretty exotic decision for model organization.
In Flutter, we are used to thinking about models as immutable entities. Because of this, all fields in models are immutable. And when one of the parameters changes, a new instance of the model is sent to the UI.
| import 'package:flutter/material.dart'; | ||
|
|
||
| class FileStatusTraillingWidget extends StatefulWidget { | ||
| final bool isLoading; |
There was a problem hiding this comment.
Using an enum may be more useful here.
enum FileStatus {
loading,
readyToLoad,
finished,
}| ), | ||
| ElevatedButton( | ||
| onPressed: () { | ||
| widget.ticketController |
There was a problem hiding this comment.
That's not a good solution. It's better not to call setState in external callbacks, as there is a risk of causing a memory leak. If you really want to do this, you can check if the widget is still mounted using the mounted property:
if (mounted) {
setState(() {
// ...
});
}
2 participants
No description provided.