-
Notifications
You must be signed in to change notification settings - Fork 131
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] Handle edge-to-edge for ApiFaker, AppSettings and MagicLink Activities #13599
[Android SDK] Handle edge-to-edge for ApiFaker, AppSettings and MagicLink Activities #13599
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.
|
.fillMaxSize() | ||
.background(color = colorResource(id = R.color.color_toolbar)) | ||
.windowInsetsPadding( | ||
WindowInsets.systemBars.only(WindowInsetsSides.Top) | ||
) |
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.
With the new handling in AppSettingsActivity, we don't need this, and we achieve the same behavior.
Also, setting the background of the Scaffold
to color_toolbar
felt off, so IMO now it's cleaner.
@@ -59,7 +59,6 @@ theme across the entire app. Overridden versions should be added to the styles.x | |||
AppBarLayout Style | |||
--> | |||
<style name="Woo.AppBarLayout" parent="Widget.MaterialComponents.AppBarLayout.Surface"> | |||
<item name="android:fitsSystemWindows">true</item> |
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.
Given that we handle the insets ourselves, we don't need this, and also when this is used, we end up with double the padding.
val systemInsets = insets.getInsets( | ||
WindowInsetsCompat.Type.systemBars() or WindowInsetsCompat.Type.displayCutout() | ||
) | ||
binding.root.doOnApplyWindowInsets(consumeInsets = true) { insets -> |
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 is no change here, I just simplified the code by using the new extension that was added in this PR.
e8cfdb0
to
2d0b4c0
Compare
…st toolbar background" This reverts commit 8969f2e.
2d0b4c0
to
a24d748
Compare
@hichamboushaba, I have two points:
|
IMO fixing the layout issues should be enough for this task, supporting edge-to-edge fully would require more effort and ideally should be validated by a designer. But, still, if we revisit the points you mentioned in this comment, I think that the fixes shipped in this PR satisfies most of them (I'm speaking specifically about the SettingsActivity, the LoginActivity is a different storey which I'll discuss below):
I think the only point that's missing is having scrollable content expanding below the navigation bar, and this will require considerable changes given how fragmented our screens, we have fragments that still use the view system and we have Composable fragments, so if we want to achieve this, we'll need to disable handling the inset at the activity level, and handle it on each fragment, which IMO should be outside of the scope here, please let me know what you think.
Thanks for sharing these, it appears these issues happen in the The Help&Support screenshot is from the HelpActivity which is not in the scope of this PR, @kidinov I see that you were planning to work on this screen, are you are aware of the above issues? |
Thanks for checking @kidinov and sorry for the noise, I'll check what changed. |
This also ensures we handle the cutout section correctly.
@irfano the last changes should have fixed all the issues that were raised:
As discussed, I will create a separate PR to fix the prologue carousel issue in trunk. And the issue with DialogFragments that you noticed in the login screen impacts the cc @kidinov |
…3350-edge-to-edge-kiwi
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
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.
Great job, @hichamboushaba! It’s well-handled across all screens. LGTM! 👍🏻
…3350-edge-to-edge-kiwi
101343e
into
13270-android-sdk-update-target-sdk-to-35
Closes: #13350
Description
This PR handles the following changes:
shadow
in the Feedback screen, and simplifies handling for some other screens like the API Faker fragment.For the other cases, I didn't do any changes:
Steps to reproduce
Testing information
The tests that have been performed
^
Images/gif
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: