-
-
Notifications
You must be signed in to change notification settings - Fork 625
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
feat(graphql_flutter): make first version of demo app #1204
base: main
Are you sure you want to change the base?
Conversation
0203e09
to
d87c8d0
Compare
d87c8d0
to
81200a7
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1204 +/- ##
==========================================
+ Coverage 59.61% 60.48% +0.86%
==========================================
Files 41 28 -13
Lines 1684 1435 -249
==========================================
- Hits 1004 868 -136
+ Misses 680 567 -113 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Vincenzo Palazzo <[email protected]>
81200a7
to
bc0b5f0
Compare
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.
Why are you deleting the tests? Also, are you sure that your example project structure actually works with pub.
|
||
class HomeViewBody extends StatelessWidget { | ||
final Logger _logger = Logger(); | ||
List<Chat> _chats = []; |
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.
We shouldn't store this as properties on a stateless widget.
} | ||
|
||
Widget _buildScrollView({required BuildContext context}) { | ||
return Query( |
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.
IMO, I think we should advocate for hooks for readability
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.
Yes, my idea was making multiple example of the same app and see how it is easy use the hooks, and also this is useful at migration time, what do you think?
return const Text("Loading chats"); | ||
} | ||
_logger.d(result.data ?? "Data is undefined"); | ||
_chats = result.parsedData as List<Chat>; |
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.
Instead of storing the data on build, we should move the query up into the main build
method and pass the data down to the children.
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.
I see, I will try to refactoring the code with this suggestion, thanks!
import 'package:graphql_flutter/graphql_flutter.dart'; | ||
import 'package:logger/logger.dart'; | ||
|
||
class HomeViewBody extends StatelessWidget { |
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.
Should we make sure our example also has mutations?
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.
Yes, this required some work to do also in the API but we could integrate it
Good catch the test are just a refused because I was trying to implement integration testing, and accidentally I removed the test dir, I will reintroduce it and also make the suggestion that you had! |
396b343
to
3628544
Compare
Continuing to split #1190
Fixes #1162
Fixes #1155
The common mistake that I make also during this demo is how we declare the link for the subscription, and I think in this case the link offers a bad API.
BTW the correct way to declare the Link in order to work with Subscriptions is the following one
Ah I was forgetting, this PR introduce also a new demo that work with the following API https://api.chat.graphql-flutter.dev based on the repository https://github.com/vincenzopalazzo/keep-safe-graphql that will be offer a lot of servers to provide integration testing server for graphql library