Issue/11750 dashboard big screen support #13488
Conversation
- Replace custom `WindowSizeClass` with `WindowWidthSizeClass` from `androidx.window.core.layout`. - Use `currentWindowAdaptiveInfo` instead custom extension that ignores screen width to calculate SizeClass - Update the number of columns in the Dashboard based on the Window Size Class. - Use a `Column` with vertical scrolling instead of `LazyColumn` when `numberOfColumns` is 1. - When multiple columns in a row instead of LazyVerticalStaggeredGrid that doesn't work well with nested scrolling interactions - Adds dependency to adaptiveAndroid
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.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| mainActivityViewModel: MainActivityViewModel, | ||
| dashboardViewModel: DashboardViewModel, | ||
| blazeCampaignCreationDispatcher: BlazeCampaignCreationDispatcher, | ||
| widgetModifier: Modifier |
Check warning
Code scanning / Android Lint
Guidelines for Modifier parameters in a Composable function Warning
WooCommerce/build.gradle
Outdated
| @@ -411,6 +411,7 @@ dependencies { | |||
| implementation(libs.androidx.compose.material.icons.extended) | |||
| implementation(libs.androidx.compose.ui.text.google.fonts) | |||
| implementation(libs.androidx.navigation.compose) | |||
| implementation(libs.androidx.adaptive.android) | |||
There was a problem hiding this comment.
Added this dependency to rely on official library to categorize screen size. We currently have a custom implmentation of WindowSizeClass but I think we should be using the official one. Also the custom implementation of WindowSizeClass currently only exposes windowHeightSizeClass but not windowWidthSizeClass. So instead of manually adding windowWidthSizeClass, I figured we could use the official size class library for it.
|
Setting the milestone to |
There was a problem hiding this comment.
I noticed that this fails:
> Task :WooCommerce:compileWasabiDebugAndroidTestKotlin FAILED
e: file:///Users/.../woocommerce-android/WooCommerce/src/androidTest/kotlin/com/woocommerce/android/e2e/screens/mystore/DashboardScreen.kt:17:37 Unresolved reference 'my_store_refresh_layout'.
Making tests execution impossible. This is caused by the removal of my_store_refresh_layout from fragment_dashboard.xml and probably can be fixed by replacing R.id.my_store_refresh_layout in DashboardScreen.kt:17:37 by another component (ideally, unique), the presence of which can inform that we're on the Dashboard Screen.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #13488 +/- ##
============================================
- Coverage 37.90% 37.88% -0.02%
Complexity 8981 8981
============================================
Files 2053 2053
Lines 112183 112254 +71
Branches 14173 14188 +15
============================================
+ Hits 42524 42532 +8
- Misses 65783 65844 +61
- Partials 3876 3878 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
@JorgeMucientes I took a liberty of updating the missing reference in UI tests with 581d2e9, so tests-wise it's good to go now
Awesome @pachlava. Thanks so much for proactively taking a look into this 🥇 |
There was a problem hiding this comment.
Really nice work @JorgeMucientes 👏, code looks good, but:
There's one thing I'm not fully convinced. To determine the number of columns, I'm using windowSizeClass.windowWidthSizeClass.
I'm not convinced either by this, the three columns looks really cramped in phones, and could also in some tablets. I also prefer using the default libraries whenever possible, but for this case, it seems the default doesn't match what we need, WDYT about going the manual way here? Instead of using the SizeClass, we can simply wrap everything in a BoxWithConstraints, and use the maxWidth property to calculate the number of columns, and we can then have a column width that looks better.
| .fillMaxSize() | ||
| .background(MaterialTheme.colors.surface) | ||
| .padding(16.dp), | ||
| numberOfColumns = when (windowSizeClass.windowWidthSizeClass) { |
There was a problem hiding this comment.
Should we coerce the numberOfColumns to the number of available cards (coerceIn(maximumValue = state.widgets.size)? For example if I have only one card, and I use the app in landscape, it will appear like this:
| override fun scrollToTop() { | ||
| return | ||
| } |
There was a problem hiding this comment.
Should we add a TODO about bringing back this functionality? This was called when re-selecting the tab, but it's broken now, as MainActivity#getActiveTopLevelFragment seems broken in trunk, I will try to investigate why later.
There was a problem hiding this comment.
Hmm, good point. I removed the nestedScrollView on XML I'm not sure how feasible would it be to fix this. Going to investigate it, if its a quick change, I'll apply it in this PR. If not, I'll create a new GH issue.
There was a problem hiding this comment.
One way would be to host the ScrollState in the ViewModel (or the Fragment if we can easily restore its state), and then pass it to the Composable code from there, and use its function (animateScrollTo or scrollTo) for triggering the scroll to top.
There was a problem hiding this comment.
So I look a bit into this, and fixing it is more complicated than I though.
I did some quick and dirty changes to try to fix shouldExpandToolbar and scrollToTop from DashboardFragment and noticed the following issues:
scrollToTop()is never called when re-selecting the tab. This happens to all the other tabs as well and it is becausegetActiveTopLevelFragment()returnsnullalways, when reselecting any tab. I don't know since when this is broken and haven't had the time to dig deper
RegardingshouldExpandToolbar,I think there's a race condition whereshouldExpandToolbar()is called when the toolbar is already expanded (too late). This causes the toolbar to always be in expanded mode when navigating back toMy Storetabs from other tabs, regardless of thescrollStatevalue.
Created a GH issue but I won't have time to look into this shortly.
The number of columns in the grid layout should not exceed the number of available widgets.
Stop relying on Android material 3 WindowSizeClass categorization since the existing values doesn't work for us leading to a cramped UI with 3 columns on phones in landscape mode.
|
Thanks for the careful review @hichamboushaba. |
There was a problem hiding this comment.
Great work 👏, this works really well, thanks @JorgeMucientes.
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Closes: #13079
Description
Based on the changes I worked on here: pe5sF9-3si-p2 and resulting in this PR work, I've refactored the prior implementation to avoid using
LazyStaggeredGridto support big screen in the dashboard tab. Instead, this relies on a custom implementation that will manually add 2 or 3 columns to the dashboard based on the screen'scurrentWindowAdaptiveInfo().windowSizeClass.Changes added:
NestedScrollView. This was needed in order to add Composable colums with vertical scroll that work well/coordinate with the collapsing toolbar.There's one thing I'm not fully convinced. To determine the number of columns, I'm using windowSizeClass.windowWidthSizeClass.
This results in bigger phones showing 3 columns in landscape (see screen recordings below). I don't think it is a major issue, but I'd say two columns would look better. Let me know what you think, although I suspect it won't be straightforward to force two columns for mobile phones on landscape mode.
Testing information
Just run the app on the phone screen and tablet screen and check that the dashboard works as expected.
The tests that have been performed
The above ☝🏼
Images/gif
Tablet Experience
TablerLayout.mp4
Large phone Eexperience (Pixel 8 pro)
LargePhone.mp4
Smaller phone experience
smallerPhone.mp4
RELEASE-NOTES.txtif 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: