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

Add onboarding screen to ask for Android permissions #7299

Merged
merged 3 commits into from
Nov 8, 2023
Merged

Conversation

cketti
Copy link
Member

@cketti cketti commented Oct 26, 2023

This removes the old onboarding module and creates several onboarding sub modules.

  • :feature:onboarding:main contains the navigation logic and binds the various onboarding modules together
  • :feature:onboarding:welcome contains the welcome screen (strings.xml files have moved; so we need to update Weblate to point to the new location)
  • :feature:onboarding:permission contains the new permission screen (needs a new component on Weblate)

image

Closes #6262

@cketti cketti requested a review from wmontwe October 26, 2023 21:45
@cketti cketti force-pushed the permission_screen branch 2 times, most recently from 61eebfb to 8ecf87d Compare October 27, 2023 19:53
@wmontwe
Copy link
Member

wmontwe commented Nov 3, 2023

I'm preparing a possible alternative for skipping this screen in case no runtime support.

@cketti cketti marked this pull request as draft November 6, 2023 13:24
@cketti cketti force-pushed the permission_screen branch from 8ecf87d to 1968e5b Compare November 6, 2023 18:46
@cketti cketti marked this pull request as ready for review November 6, 2023 23:14
Copy link
Member

@wmontwe wmontwe left a comment

Choose a reason for hiding this comment

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

I had a closer look and was wondering why there are no tests. Despite that it looks good and I just had some findings.

The bottombar buttons need more bottom spacing.

Also the Feature App needs the contact permisison to not fail instantly. That's where I noticed that there is not enough visual feedback when the check failed. I felt a little bit lost and just noticed that the icon changed after clicking the allow button multiple times.

onBack: () -> Unit,
onFinish: (String) -> Unit,
) {
composable(route) {
Copy link
Member

Choose a reason for hiding this comment

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

I think the nested route should be declared directly in the OnboardingNavHost to prevent unnecessary coupling.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I follow. Are you saying it's better for the interface between :onboarding:main and :account:setup to be AccountSetupScreen() rather than nestedAccountSetupRoute()? The first one feels like an implementation detail to me. And one that might change in the future (to use navigation instead of manually handling sub screens).

Copy link
Member

Choose a reason for hiding this comment

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

My issue is that nestedAccountSetupRoute() only existis because of the needs of the onboarding implementation to use a different entry point.

Copy link
Member

@wmontwe wmontwe left a comment

Choose a reason for hiding this comment

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

LGTM

@cketti cketti force-pushed the permission_screen branch from 9b1d649 to fe33dd2 Compare November 8, 2023 14:23
@cketti cketti merged commit eb7dcf2 into main Nov 8, 2023
2 checks passed
@cketti cketti deleted the permission_screen branch November 8, 2023 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ask for contacts permission during onboarding
2 participants