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

Enforced edge-to-edge #1078

Open
wants to merge 14 commits into
base: main-ose
Choose a base branch
from

Conversation

ArnyminerZ
Copy link
Member

@ArnyminerZ ArnyminerZ commented Oct 16, 2024

Purpose

See #1077

Short description

  • Replaced the deprecated Window.statusBarColor with edge-to-edge settings.
  • Set WindowInsets to 0 in Assistant since we are using an action bar outside of an Scaffold, and the padding is being applied twice.
  • IME padding is applied automatically
Intro Screen 1 Intro Screen 2 Main Screen
edgeToEdge_light-intro-34 edgeToEdge_light-intro2-35 edgeToEdge_light-main-34
edgeToEdge_dark-intro-34 edgeToEdge_dark-intro2-35 edgeToEdge_dark-main-34

Checklist

  • The PR has a proper title, description and label.
  • I have self-reviewed the PR.
  • I have added documentation to complex functions and functions that can be used by other modules.
  • I have added reasonable tests or consciously decided to not add tests.

@ArnyminerZ ArnyminerZ linked an issue Oct 16, 2024 that may be closed by this pull request
2 tasks
@ArnyminerZ ArnyminerZ self-assigned this Oct 16, 2024
@ArnyminerZ ArnyminerZ added the bug Something isn't working label Oct 16, 2024
@ArnyminerZ ArnyminerZ marked this pull request as ready for review October 16, 2024 09:10
@ArnyminerZ ArnyminerZ requested review from sunkup and removed request for sunkup October 16, 2024 09:10
@ArnyminerZ ArnyminerZ marked this pull request as draft October 16, 2024 09:13
@ArnyminerZ ArnyminerZ marked this pull request as ready for review October 16, 2024 11:02
@rfc2822 rfc2822 added enhancement New feature or request and removed bug Something isn't working labels Oct 16, 2024
@rfc2822 rfc2822 requested review from sunkup and removed request for rfc2822 October 20, 2024 15:08
Copy link
Member

@sunkup sunkup left a comment

Choose a reason for hiding this comment

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

Needs some adjustments.

sunkup
sunkup previously approved these changes Oct 22, 2024
Copy link
Member

@sunkup sunkup left a comment

Choose a reason for hiding this comment

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

Yes, now it works 👍

We should probably implement proper edge-to-edge support soon. All good.

@sunkup sunkup requested a review from rfc2822 October 22, 2024 09:39
@ArnyminerZ
Copy link
Member Author

We should probably implement proper edge-to-edge support soon.

Do you mean like merging this or there is something missing on this PR? I mean, it should already implement full edge-to-edge support.

@sunkup
Copy link
Member

sunkup commented Oct 22, 2024

We should probably implement proper edge-to-edge support soon.

Do you mean like merging this or there is something missing on this PR? I mean, it should already implement full edge-to-edge support.

Right, my bad. I used the wrong emulator once. Forget it :)

devvv4ever
devvv4ever previously approved these changes Oct 25, 2024
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

Changed a view things:

  1. Theme: The transparent navigation bar was not good on Android <15, the buttons were white on very light grey. I think it's ok like that on Android <15.
  2. Intro pages: I have used "safe drawing" and consumed top+sides (like TopAppBar) instead of only the status bar padding for the intro pages content, and bottom+sides (like BottomAppBar) for the navigation bar. As I understand it, we shouldn't use only navigation and status bar insets and ignore the other ones, because they may have relevance in some situations. / Also removed the obsolete 90 dp Spacer.
  3. Assistant: made clear that it should be used in a Scaffold that handles insets.

@ArnyminerZ Please have a look at the changes + check whether these are working for you. Tested with Android 7.1.1 and 15 in light+dark mode and looked fine here and maybe there are still duplicate/missing insets or so.

@ArnyminerZ
Copy link
Member Author

I've tried it with Android 14, and there are some odd things. First of all, the intro looks great, but on the home screen drawer, the bottom part is perfect, but I think the top part should be "through the status bar":

Screenshot

side nav

It's kind of cut-off. Then, when adding an account, the navigation bar color should be lighter I think:

Screenshot

bottom bar color

As well as in the settings, we can leave it as is if we want that functionality, but right now the content is behind the navigation bar, and not showing through like it theoretically should with edge-to-edge:

Screenshot

settings bottom overlap

@rfc2822
Copy link
Member

rfc2822 commented Oct 27, 2024

[…] but I think the top part should be "through the status bar":

To be honest, it's like I had it in mind:

--- white background ---
--- branding box on dark background (originally not intended to look like a TopAppBar) ---
--- white again / content ---

You mean that the dark background of the branding should completely go to the top as in the TopAppBars, right? Yes, why not. We can do it like that, maybe it looks more consistent with the app bars.

Then, when adding an account, the navigation bar color should be lighter I think:

Agreed, can you change it?

As well as in the settings, we can leave it as is if we want that functionality, but right now the content is behind the navigation bar, and not showing through like it theoretically should with edge-to-edge:

Ah I didn't even notice that it should shine through as it doesn't really make sense to me that the content of an app shines through system controls. But that's the whole concept of edge-to-edge, and it seems that it's intended as you say, so yes, I'm for changing that too.

Signed-off-by: Arnau Mora Gras <[email protected]>
Signed-off-by: Arnau Mora Gras <[email protected]>
Signed-off-by: Arnau Mora Gras <[email protected]>
Signed-off-by: Arnau Mora Gras <[email protected]>
Signed-off-by: Arnau Mora Gras <[email protected]>
@rfc2822 rfc2822 force-pushed the 1077-edge-to-edge-not-working-properly branch from f13602e to f28edc9 Compare November 11, 2024 09:20
@rfc2822
Copy link
Member

rfc2822 commented Nov 12, 2024

So still to do:

  • Branding box extends to upper edge
  • Set correct navigation bar color, if applicable
  • Let content shine through navigation bar / especially for lists like settings

@rfc2822 rfc2822 dismissed stale reviews from sunkup and devvv4ever November 12, 2024 14:15

Not ready yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Edge to Edge not working properly
4 participants