Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat/ add loading screen for SKS Menu #427
Changes from 9 commits
da37b5e
ebf6869
0f3599b
73eb38e
97ba4cd
ef8e97c
1fe2fca
0a99fb0
15f17fa
676a32e
95a35a6
46f3d82
a514fe7
26a5859
3860274
257b04e
e309efa
90e1bcb
2f1600f
a656a76
4bed94a
45a73d2
ffe9c55
10ce488
75be777
bc9cda5
750ba0f
402eba4
13c82d8
5dd25a7
848e896
cfce4ba
9d8468a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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
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.
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 checkScrollableLoaderBuilder
to determine how many items need to be displayed