Skip to content
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.

Sign up for GitHub

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

[2 pane layout] Tablet mode detection logic change #13228

Merged
merged 10 commits into from
Jan 30, 2025
Merged

[2 pane layout] Tablet mode detection logic change #13228

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b45041b
Change the window size detection logic so that we consider both width…
AnirudhBhat Dec 31, 2024
62428af
Remove needless blank line
AnirudhBhat Dec 31, 2024
e0b9d62
Add release notes
AnirudhBhat Jan 2, 2025
f43bda8
Remove mixing of window size logic and our custom two pane layout logic
AnirudhBhat Jan 28, 2025
f4e100c
Fix detekt errors
AnirudhBhat Jan 28, 2025
e74ef96
Merge branch 'refs/heads/trunk' into issue/12861-windowsize-detection…
AnirudhBhat Jan 28, 2025
3f8497f
Remove unused imports and add necessary import
AnirudhBhat Jan 28, 2025
dd61dda
Update release notes
AnirudhBhat Jan 28, 2025
91c4324
Rename variable names used in analytics for better understanding.
AnirudhBhat Jan 30, 2025
7e2c9bc
Merge branch 'refs/heads/trunk' into issue/12861-windowsize-detection…
AnirudhBhat Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions RELEASE-NOTES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
- [*] Fixed overlap issue in Settings > WooCommerce Version [https://github.com/woocommerce/woocommerce-android/pull/13183]
- [*] Fixed a crash on the order details [https://github.com/woocommerce/woocommerce-android/pull/13191]
- [**] Fixed a crash when a shop manager was trying to install or activate plugin in the POS onboarding [https://github.com/woocommerce/woocommerce-android/pull/13203]
- [*] Refactored tablet detection logic to consider both width and height dimensions, enhancing compatibility with larger phones and tablets, particularly in landscape mode [https://github.com/woocommerce/woocommerce-android/pull/13228]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

np: I think this is not a refactoring, but change of the logic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated here: dd61dda


21.3
-----
Expand Down
20 changes: 16 additions & 4 deletions WooCommerce/src/main/kotlin/com/woocommerce/android/extensions/ContextExt.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,29 @@ import androidx.annotation.ColorRes
import androidx.core.content.ContextCompat
import com.woocommerce.android.util.SystemVersionUtils
import kotlinx.parcelize.Parcelize
import kotlin.math.max
import kotlin.math.min

val Context.windowSizeClass: WindowSizeClass
get() = determineWindowWidthSizeClassByGivenSize(resources.configuration.screenWidthDp)
get() = determineWindowSizeClassByDimensions(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AnirudhBhat I think we can not use the same semantics here if we change the logic. Window classes we use here are from https://m3.material.io/foundations/layout/applying-layout/window-size-classes and those are determined based on the width of a device. Reusing the same concept but with different logic may bring confusion and errors

I am thinking that:

  • We should either simplify all of this and end up with something "isTwoPaneShouldBeUsed" with logic similar/the same as WooPosIsScreenSizeAllowed. I'd say this is preferred, as I think we learned that the use of those size classes doesn't work well at all
  • Or at least refactor all of this and call it in not attaching to the material design size classes name (remove comments etc) to make sure that we don't mix the concepts

wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late response.

I agree with your opinion that mixing Window classes and our custom detection logic would create confusion. I've updated the logic to remove Window class reference. Please take a look.

f43bda8

resources.configuration.screenWidthDp,
resources.configuration.screenHeightDp
)

val Context.windowHeightSizeClass: WindowSizeClass
get() = determineWindowHeightSizeClassByGivenSize(resources.configuration.screenHeightDp)

private fun determineWindowWidthSizeClassByGivenSize(sizeDp: Int): WindowSizeClass {
private fun determineWindowSizeClassByDimensions(widthDp: Int, heightDp: Int): WindowSizeClass {
val shortSize = min(widthDp, heightDp)
val longSize = max(widthDp, heightDp)

return when {
sizeDp < WindowSizeClass.Compact.maxWidthDp -> WindowSizeClass.Compact
sizeDp < WindowSizeClass.Medium.maxWidthDp -> WindowSizeClass.Medium
shortSize < WindowSizeClass.Compact.maxWidthDp || longSize < WindowSizeClass.Compact.maxHeightDp -> {
WindowSizeClass.Compact
}
shortSize < WindowSizeClass.Medium.maxWidthDp || longSize < WindowSizeClass.Medium.maxHeightDp -> {
WindowSizeClass.Medium
}
else -> WindowSizeClass.ExpandedAndBigger
}
}
Expand Down
Loading