Feat/add lottie animation #428
Conversation
|
It appears that again some weird linter error occurred. Imo this happens only if build_runner has not been sent before push. And sure, I agree, task with offline menu should be created separately (since directed for beginners). I will create one and assign it for you, as a task for next week ;) |
|
That wasn't the whole review and I'm not an expert in android build tools, but I'm pretty sure that it's not proper to do this changes |
|
I think @mikolaj-jalocha can say more it that matter |
There was a problem hiding this comment.
First of all, congrats of your first commit here and working code! Animation looks great😎
I left few suggestions of mine regarding UI. I think button could be removed (as of now) and some text regarding error could be displayed as well.
I also left notes regarding better use of lottie and some regarding changes of files that should only be made locally.
lib/features/sks-menu/presentation/widgets/sks_menu_loading.dart
Outdated
Show resolved
Hide resolved
lib/features/sks-menu/presentation/widgets/sks_menu_loading.dart
Outdated
Show resolved
Hide resolved
lib/features/sks-menu/presentation/widgets/sks_menu_loading.dart
Outdated
Show resolved
Hide resolved
lib/features/sks-menu/presentation/widgets/sks_menu_loading.dart
Outdated
Show resolved
Hide resolved
lib/features/sks-menu/presentation/widgets/sks_menu_loading.dart
Outdated
Show resolved
Hide resolved
lib/features/sks-menu/presentation/widgets/sks_menu_loading.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
So good job and congrats for the first completed task.
I've left one note about the sized box, but everything else was pretty much covered by Tomek and Mikołaj, so good job guys too.
As for java build settings, let's not change anything globally for now. I've actually seen that Andrea has made some fancy tool related on the topic, so someone can investigate on that: https://gist.github.com/bizz84/605e2ca2088cb4acb7a076ca993f41cd?trk=public_post_comment-text But let's rollback for now
lib/features/sks-menu/presentation/widgets/sks_menu_loading.dart
Outdated
Show resolved
Hide resolved
|
Sorry for pushing java path etc., @mikolaj-jalocha was right (some demands of Android Studio), will not happen again. |
There was a problem hiding this comment.
Name of the file could be different since this animation's not responsible for loading. In fact we could use that class inside top level sks_menu_screen as a private class.
| ), | ||
| ), | ||
| Align( | ||
| child: Text( |
There was a problem hiding this comment.
Maybe it'd be good to add detailed error message (error message that comes from api). Maybe in small print and in different color.
| }); | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { |
There was a problem hiding this comment.
It'd be great to add logger somewhere here:
Logger().e(error.toString());
| @@ -63,11 +56,7 @@ class _SksMenuView extends StatelessWidget { | |||
| @override | |||
| Widget build(BuildContext context) { | |||
| if (sksMenuData.meals.isEmpty) { | |||
There was a problem hiding this comment.
Here we should check if menu's online or not and then show animation (list of meals now is only empty in case of error, if sks's closed cached data are provided, so this condition'll never work)
… and adding sks_old_menu text
simon-the-shark
force-pushed
the
feat/add-lottie-animation
branch
from
9187dba to
d44cab9
Compare
* feat: add lottie animation for sks menu * feat: adding statefull widget for showing the menu from the latest update * feat: changing to CircularProgressIndicator() for loading of sks menu and adding sks_old_menu text * feat: add sks menu loading * feat: sks menu not available message * chore: fix linter complaints * feat: show error on screen --------- Co-authored-by: mikolaj-jalocha <[email protected]>
When error occurres or SKS's closed, the lottie animation "closed" is showing on SKS Menu.
In my opinion, the offline menu functionality is out of scope for this task and should be addressed as a separate task.