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.

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

[Android SDK] Edge to edge SettingsActivity #13469

Open
wants to merge 11 commits into
base: 13270-android-sdk-update-target-sdk-to-35
Choose a base branch
from

Conversation

kidinov
Copy link
Contributor

@kidinov kidinov commented Feb 5, 2025

part: #13350

Description

The PR adapts App Settings Activity for edge to edge enabled due to target sdk 35

Testing information

  • Use tablets and phones on android 35 and below
  • Use dark and light modes
  • More -> Settings
  • Click through all subscreens
  • Notice that it looks as before, except the toolbar looks like "edge to edge" now

The tests that have been performed

Above

Images/gif

  • I have considered if this change warrants release notes and have added them to 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:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

@kidinov kidinov added status: do not merge Dependent on another PR, ready for review but not ready for merge. Tech Debt labels Feb 5, 2025
@kidinov kidinov added this to the 21.7 milestone Feb 5, 2025
@kidinov kidinov requested a review from irfano February 5, 2025 13:38
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Feb 5, 2025

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App Name WooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commit79d018e
Direct Downloadwoocommerce-wear-prototype-build-pr13469-79d018e.apk

@kidinov kidinov changed the title 13350 edge to edge app settings activity [Android SDK] Edge to edge SettingsActivity Feb 5, 2025
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Feb 5, 2025

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App Name WooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commit79d018e
Direct Downloadwoocommerce-prototype-build-pr13469-79d018e.apk

@irfano irfano self-assigned this Feb 6, 2025
@irfano
Copy link
Contributor

irfano commented Feb 6, 2025

I haven’t looked into this enforcement topic before, but it seems to require a lot of effort. I’ll share some screenshots where I noticed issues. I’ll continue my review.


Base automatically changed from 13463-android-sdk-revisit-activities-to-see-if-landscape-3-button-navigation-handled-well to 13270-android-sdk-update-target-sdk-to-35 February 7, 2025 10:19
@kidinov kidinov removed the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Feb 7, 2025
@kidinov
Copy link
Contributor Author

kidinov commented Feb 7, 2025

@irfano 👋

System status report It's another activity, and I believe it's been addressed already in the target branch

Tbh, I don't see any issues with the rest of the screenshots. You mean the hole is on top of the layout? I think this is how it is supposed to be. Check screenshots:

https://developer.android.com/develop/ui/views/layout/edge-to-edge

@wpmobilebot wpmobilebot modified the milestones: 21.7, 21.8 Feb 7, 2025
@wpmobilebot
Copy link
Collaborator

Version 21.7 has now entered code-freeze, so the milestone of this PR has been updated to 21.8.

@irfano
Copy link
Contributor

irfano commented Feb 7, 2025

I can't read texts behind the camera hole. I think there should be a margin on the left side.

I found a similar example in Google documents. It states that we should apply insets so that UI is not hidden.

Important: If your app is not already edge-to-edge, portions of your app may be obscured and you must handle insets.

image

I think the second screenshot here shows how it’s supposed to be. Wdyt?

@kidinov
Copy link
Contributor Author

kidinov commented Feb 11, 2025

@irfano

Thanks for pointing this out! I missed that.

I think this is quite a strange decision by Google. In the case of the navigation bar, they state that the content should be underneath it so it's only accessible by scrolling up. However, in this instance, where the destruction area is much, much smaller, they suggest cutting out a big chunk of the screen while the content is accessible by scrolling. I don’t understand their logic here 🙈

Anyway, I believe they have their reasons, so I adjusted it here. Please take a look. Additionally, for the main activity, there is the same issue. It looks rather questionable to my taste =)

dd90c0c

image image

@codecov-commenter
Copy link

codecov-commenter commented Feb 11, 2025

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 41.36%. Comparing base (c052c7b) to head (63762cf).

Files with missing lines Patch % Lines
...tlin/com/woocommerce/android/extensions/ViewExt.kt 0.00% 4 Missing ⚠️
...ce/android/ui/main/MainActivityEdgeToEdgeHelper.kt 0.00% 3 Missing ⚠️
Additional details and impacted files
@@                             Coverage Diff                              @@
##             13270-android-sdk-update-target-sdk-to-35   #13469   +/-   ##
============================================================================
  Coverage                                        41.36%   41.36%           
- Complexity                                        6579     6580    +1     
============================================================================
  Files                                             1332     1332           
  Lines                                            77900    77902    +2     
  Branches                                         10724    10724           
============================================================================
+ Hits                                             32225    32228    +3     
  Misses                                           42809    42809           
+ Partials                                          2866     2865    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…dge-to-edge-AppSettingsActivity

# Conflicts:
#	WooCommerce/src/main/kotlin/com/woocommerce/android/ui/main/MainActivity.kt
#	WooCommerce/src/main/kotlin/com/woocommerce/android/ui/main/MainActivityEdgeToEdgeHelper.kt
@irfano
Copy link
Contributor

irfano commented Feb 12, 2025

Let me try to explain my understanding of the edge-to-edge feature:

  • Background colors or images should extend behind the system bars. This should be evaluated for each screen individually.
  • If the content is scrollable, it should be visible behind the system bars. However, when scrolling to the end of the list, the last item should remain fully visible.
  • In landscape orientation, vertical scrolling occurs from the bottom. The content can extend behind the bottom system bar, but the left system bar should not cover any part of the content.

Let’s take the Calendar app as an example. Currently, the PR does not behave the same way as the Calendar app, but we can change that. I think the only difference is the top and bottom system bars in landscape orientation. I’d love to hear your thoughts.

@kidinov
Copy link
Contributor Author

kidinov commented Feb 13, 2025

@irfano 👋

Your understanding matches my. And I am trying to take a balanced approach here without changing too much in every fragment and at the same time to end up with an "as it was or better".

If you have an idea how to archive the ideal state with minimum risk/changes, please suggest 🙏

@irfano
Copy link
Contributor

irfano commented Feb 15, 2025

@kidinov, I looked into this, and it seems there isn't a solution like using single function to handle all screens. The initial state of this PR wasn’t ideal since Google explicitly stated that the UI shouldn’t be hidden behind the camera. I tested the latest commit on Android 15 and Android 9 devices and I confirmed that it works as before.
I couldn't follow the previous discussion about this topic, but I see two options

  • Support edge-to-edge on all screens properly. This approach may require different solutions for each activity since some Material views automatically adapt to edge-to-edge while others don’t.
  • Merge this PR, open an issue, and support edge-to-edge later. This way, we can support edge-to-edge on some screens, such as those using newer Compose components, which are easier to support.

WDYT? I think you're leaning toward the latter option, so I'd suggest opening an issue for "Edge-to-edge support of AppSettingsActivity".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants