-
Notifications
You must be signed in to change notification settings - Fork 136
[Blaze] Fix spacing issue in Blaze Campaigns dashboard card #13300
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
Conversation
…ze Campaigns card
📲 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.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #13300 +/- ##
============================================
- Coverage 41.10% 41.10% -0.01%
+ Complexity 6421 6420 -1
============================================
Files 1321 1321
Lines 77177 77177
Branches 10643 10644 +1
============================================
- Hits 31722 31721 -1
Misses 42646 42646
- Partials 2809 2810 +1 ☔ View full report in Codecov by Sentry. |
JorgeMucientes
left a comment
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.
Nicely done @hafizrahman, good use of FlowRow.
I left a minor suggestion and also have a question. Is this really a beta fix? I feel its not that critical, but not sure if this was discussed elsewhere.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/blaze/campaigs/BlazeCampaignItem.kt
Outdated
Show resolved
Hide resolved
My thought was that:
I'm fine with adding this to the next version, though, I just thought it'd be nice to ship it earlier and I suppose it won't be too time consuming to test before release. |
…ampaigs/BlazeCampaignItem.kt Co-authored-by: Jorge Mucientes <[email protected]>
|
Thanks for sharing your thoughts @hafizrahman 🙂. My 2 cents around that:
That being said, the change is small and safe. I don't want to block this PR or anything, so I'm good if you prefer merging this on the release branch 👍🏼. |
Yeah, good point actually. Let's just bring it for the next release 👍🏼 |
Closes: #13298
Description
In the Blaze Campaigns dashboard card, the "Total" column might disappear if the combination of font size and display size setting on the device makes everything big.
The fix proposed in the PR makes use of
FlowRowto make the two columns responsive. They remain the same on smaller font and display size, but on bigger ones, the "Total" field will be pushed on its own row, keeping it readable at all sizes.Previously:
With this PR:
Additionally, due to the
FlowRowusage, this now brings a change in landscape/wider mode in general, where the two items will now be separated. I feel this should not be an issue since they are still readable:Steps to reproduce
Testing information
n/a
The tests that have been performed
I tested the above in landscape and portait mode on API 35
Images/gif
Included above.
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: