-
Notifications
You must be signed in to change notification settings - Fork 2
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/ add loading screen for SKS Menu #427
Conversation
BombardierBulge
commented
Nov 24, 2024
•
edited
Loading
edited
- replaced random animation loading indicator's with shimmer loading animation
- look preview
* Feat: create sks menu screen - prototype * feat: create sks menu screen * feat(sks-menu): improve spacings and font * fix(sks-menu): fix sks url link * fix(sks-menu): remove bottom nav bar * feat (sks-menu): add data layer * chore (sks-menu): code cleanup * feat (sks-menu): pr fix --------- Co-authored-by: Szymon Kowaliński <[email protected]>
…c. (#414) Co-authored-by: Rafal Rejek <[email protected]>
* feat: add interval refresh on the sks people counter * refactor: add separate files for refresh on ref and consts add files: sks_people_live_consts.dart ref_extensions.dart change files: home_viev.dart: add SksAppBar to AppBar parkings_repository.dart, latest_sks_user_data_repository.dart: refactor code * refactor: delete unused imports
# Conflicts: # lib/features/navigator/app_router.dart # lib/features/sks-menu/data/models/sks_menu_data.dart # lib/features/sks-menu/data/repository/sks_menu_repository.dart # lib/features/sks-menu/presentation/sks_menu_screen.dart # lib/l10n/app_pl.arb
@BombardierBulge please fix linter complaints (instructions in readme) and if possible please attach some screenshots. When you're ready please request review from me or @tomasz-trela. Good job! |
lib/features/sks-menu/presentation/widgets/sks_menu_view_loading.dart
Outdated
Show resolved
Hide resolved
d1b97ce
to
1fe2fca
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.
In the comparison with the first version code now looks extremely simple and clean. So, congrats on that.
In my opinion some parts of UI could be slightly better mapped into shimmer loading, but don't waste time on it..
Please see my comments about calculations (important) and private classes.
@@ -49,3 +51,21 @@ class SksMenuHeader extends StatelessWidget { | |||
); | |||
} | |||
} | |||
|
|||
class SksMenuHeaderLoading 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.
This class could be private.
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.
This class could be private and moved to sks_menu_view_loading
@@ -74,3 +75,41 @@ class SksMenuDishDetailsTile extends StatelessWidget { | |||
); | |||
} | |||
} | |||
|
|||
class SksMenuTilesLoading 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.
This class could be private.
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.
This class could be private and moved to sks_menu_view_loading
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.
There was a problem accessing context.colorTheme.whiteSoap when SksMenuTilesLoading was moved to sks_menu_view_loading. How can I fix this?
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.
White soap here is optional I guess (since we re using shimmer) , you can use whatever colour you prefer or use Colour consts (see my commit with sks menu or draft commit with chart)
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.
100% certain. Colour here could be even pink, it completely doesnt matter
} | ||
} | ||
|
||
class LoadingTitle 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.
This class could be private
lib/features/sks-menu/presentation/widgets/sks_menu_view_loading.dart
Outdated
Show resolved
Hide resolved
separatorBuilder: (context, _) => const SizedBox( | ||
height: 8, | ||
), | ||
itemCount: 3, |
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 think we should made some kind of calculations to ensure that large screens are not in 80% blank.
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 i use MediaQuery.of(context).size.height or SksMenuConfig padding value in ui_config ?
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.
Height here is ok. itemCount
is what we re discussing. Please check ScrollableLoaderBuilder
to determine how many items need to be displayed
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.
About the calculations etc, I would reccomend checking out and reusing our ScrollableLoaderBuilder
widget in some way @mikolaj-jalocha @BombardierBulge
* feat: caching_rest_response * chore/ format code * refactor/parametrize function * feat: condition * refactor/ meet review requirements * chore/ format code * feat: caching_rest_response * chore/ format code * refactor/parametrize function * feat: condition * refactor/ meet review requirements * chore/ format code * refactor/ unify rest clients * feat/ aditional comments and renaming
…eing checked recently
…ks website & adapt for new data format
navigation_tab_view.dart: add revirew openStoreListing behavior to review button
* Add Wrap * Fix translation * Fix problem with Dialog on small screens --------- Co-authored-by: Rafal Rejek <[email protected]>
* 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]>
* feat: navgation aware back button Co-authored-by: Szymon Kowaliński <[email protected]> * refactor: delete unused imports * fix(navigator): resolve race hazard related to view transistions duration --------- Co-authored-by: piwniczak2137 <[email protected]>
* /fix fix shadows on filter's screen * /fix fix shadows on filter's screen
What's the status on the PR? Did you check out the |
Unfortunately no, but I'll check it soon in free time. |
What's the status here? |
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.
Final touches ;)
@@ -74,3 +75,41 @@ class SksMenuDishDetailsTile extends StatelessWidget { | |||
); | |||
} | |||
} | |||
|
|||
class SksMenuTilesLoading 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.
This class could be private and moved to sks_menu_view_loading
@@ -49,3 +51,21 @@ class SksMenuHeader extends StatelessWidget { | |||
); | |||
} | |||
} | |||
|
|||
class SksMenuHeaderLoading 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.
This class could be private and moved to sks_menu_view_loading
} | ||
} | ||
|
||
class _SksMenuTitleLoadingPaddings 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.
I don't like the name here. We don't use such convention anywhere else. Probably better to change it to SksMenuTileLoading
and merge with existing widget of that name
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.
Please see the comments
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.
EXCELLENT CODE
LGTM 🚀🚀🚀🚀