-
Notifications
You must be signed in to change notification settings - Fork 137
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
Dashboard big screen support with staggered grid layout for cards #13079
base: trunk
Are you sure you want to change the base?
Conversation
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
mainActivityViewModel: MainActivityViewModel, | ||
dashboardViewModel: DashboardViewModel, | ||
blazeCampaignCreationDispatcher: BlazeCampaignCreationDispatcher, | ||
widgetModifier: Modifier |
Check warning
Code scanning / Android Lint
Guidelines for Modifier parameters in a Composable function Warning
@@ -362,7 +349,11 @@ class DashboardFragment : | |||
) | |||
} | |||
|
|||
override fun shouldExpandToolbar() = binding.statsScrollView.scrollY == 0 | |||
override fun shouldExpandToolbar() = true |
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 is a small issue I'm not sure it can be resolved with the current implementation. This callback handles showing the toolbar collapsed/expanded properly when navigating back and forth from dashboard. Using LazyColumn or LazyStaggered grid we don't have access to scroll position and thus we won't be able to tell if the toolbar should be expanded or collapsed. By default we'll show it expanded and as soon as the user scrolls it will adjust accordingly:
toolbarExpandedDefault.mp4
I think this is an acceptable tradeoff if we get proper support for tablet screens.
Closes: #13079
Description
An attempt to support big screens for dashboard Tab. I simplified the xml layout and refactored the implementation to use LazyColumn for smaller screens and LazyStaggeredGrid for big screen. The problem is LazyStaggeredGrid is bugged in several way and the support
Changes added:
NestedScrollView
. This was needed in order to add Lazy column and grid and not run into nested scroll issue:I will go deeper into the issues in P2 post:
Phone screen using Lazy column works like a charm:
phoneWorksWell.mp4
Tablet screen using LazyStaggered grid breaks the pull to refresh implementation as well as the collapsing toolbar. Pitty the component is bugged:
IssuesWithScrollInStaggeredGrids.mp4
Steps to reproduce
Testing information
The tests that have been performed
Images/gif
RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: