-
Notifications
You must be signed in to change notification settings - Fork 132
[Orders] UI improvements for small tablets #14152
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
[Orders] UI improvements for small tablets #14152
Conversation
📲 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 #14152 +/- ##
============================================
- Coverage 37.78% 37.78% -0.01%
- Complexity 8945 8946 +1
============================================
Files 1956 1956
Lines 109561 109566 +5
Branches 14385 14387 +2
============================================
+ Hits 41396 41397 +1
- Misses 64388 64393 +5
+ Partials 3777 3776 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@@ -108,6 +110,7 @@ class OrderDetailFragment : | |||
companion object { | |||
val TAG: String = OrderDetailFragment::class.java.simpleName | |||
private const val MARGINS_FOR_TABLET: Float = 0.1F | |||
private const val MARGINS_FOR_SMALL_TABLET: Float = 0.01F |
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 believe we should use something like 16dp in this instance; otherwise, it may not adhere to the 4dp grid guideline and become noticeable as this is small padding
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.
Makes sense. Set to fixed 16dp: f2a761d
@@ -97,7 +97,7 @@ class OrderListFragment : | |||
const val FILTER_CHANGE_NOTICE_KEY = "filters_changed_notice" | |||
|
|||
private const val JITM_FRAGMENT_TAG = "jitm_orders_fragment" | |||
private const val TABLET_LANDSCAPE_WIDTH_RATIO = 0.3f | |||
private const val TABLET_LANDSCAPE_WIDTH_RATIO = 0.4f |
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.
While it's name "landscape" it's actually set 0.4 to both Portrait and landscape orientations. So:
- Maybe name it properly
- It feels that 0.3 is better for landscape, but 0.4 for portrait
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 hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to .4 in portrait and .3 in landscape: a3d096f
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.
LGTM! I left a couple of suggestions please take a look
Also, I'll leave this up to you, but now, in some cases we show 2 panes in portrait and 1 pane in landscape, which is odd
06-09--11-57.mp4
Thanks for review, @kidinov.
Good catch. Since this is a separate bug, and may require some more tests and discussion, I raised a separate PR with the fix: #14160. You can take a look. |
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.
👋 @kidinov, I can't reproduce such large margins on a similar configuration. Could you check the actual dp size of the screen, for example, by measuring the dp size of the root container using the layout inspector in AS? It's the dp value of the shorter dimension that is being classified in I suspect your device has larger dp screen width class than |
I just realised that I tested it on the products screen, not orders. Sorry about that. We'd need to implement this on the products though, otherwise it looks inconsistent. |
Fix support for large font scale size on small tablets
WOOMOB-537
Description
This PR adds a couple of improvements for Orders screen on small tablets, especially with increased size of font and increased pixel density:
Steps to reproduce
Testing information
Check if you can reproduce any UI glitches with accessibility size settings (screen size, font size).
The tests that have been performed
Tested Orders | Order details screen on different devices and configurations:
Results below.
Images/gif
RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.