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

Replace AppIntro by Compose Pager #848

Merged
merged 13 commits into from
Jun 25, 2024

Conversation

ArnyminerZ
Copy link
Member

The PR should be in Draft state during development. As soon as it's finished, it should be marked as Ready for review and a reviewer should be chosen.

See also: Writing A Great Pull Request Description

Purpose

Migrates all AppIntro code to Compose Pager.

Short description

  • Added ButtonWithIcon which is a circular button with an icon inside.
  • Added PositionIndicator which shows multiple circles indicating the current position of the slider.
  • Added IntroScreen which handles all the things AppIntro did before.
  • Got rid of AppIntro dependency

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 added the enhancement New feature or request label Jun 11, 2024
@ArnyminerZ ArnyminerZ self-assigned this Jun 11, 2024
@ArnyminerZ ArnyminerZ linked an issue Jun 11, 2024 that may be closed by this pull request
@rfc2822 rfc2822 requested review from rfc2822 and sunkup June 11, 2024 19:40
@mbiebl
Copy link
Contributor

mbiebl commented Jun 14, 2024

Would probably be a good idea to mention in the git log message why AppIntro was replaced/removed.

Two smaller issues:

  • fails to build as the code still references appintro:
$ grep appintro app/src/ -R
app/src/main/kotlin/at/bitfire/davdroid/ui/intro/WelcomePage.kt:                            com.github.appintro.R.dimen.appintro2_bottombar_height
app/src/main/kotlin/at/bitfire/davdroid/ui/intro/WelcomePage.kt:                            com.github.appintro.R.dimen.appintro2_bottombar_height
  • Small typo in the commit message: "Migrated into to compose". This probably should read "Migrated Intro to compose"

@ArnyminerZ ArnyminerZ marked this pull request as ready for review June 15, 2024 06:56
@rfc2822 rfc2822 removed their request for review June 16, 2024 17:06
@sunkup sunkup removed their request for review June 17, 2024 07:57
@sunkup
Copy link
Member

sunkup commented Jun 17, 2024

@ArnyminerZ Please request a review from me again when ready :)

@sunkup sunkup marked this pull request as draft June 17, 2024 07:58
@ArnyminerZ ArnyminerZ marked this pull request as ready for review June 17, 2024 09:22
@ArnyminerZ ArnyminerZ requested a review from sunkup June 17, 2024 09:22
@ArnyminerZ
Copy link
Member Author

Should be

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.

Looking good. Minor comments :)

@ArnyminerZ ArnyminerZ requested a review from sunkup June 17, 2024 11:26
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.

Looking forward to remove the last non-Compose code :) Noticed some things:

  1. When you click through the pages (for instance, only 2 pages like in the video), DAVx5 quits instead of going to the Accounts overview:
Screen_recording_20240623_105103.webm
  1. I think it would look better if the position indicator circles would always have the same distance to each other.

Old app intro:

Screenshot_20240623_104903

In contrast, the new app intro with the big distance between the circles:

Screenshot_20240623_105037

I think we should define a fixed dp distance between the circles.

  1. I'd leave ButtonWithIcon and PositionIndicator in the screen where it's needed. We don't need it somewhere else and it makes the composable list long and at some time confusing. When we at some time need it somewhere else, we can extract it to the composables package.

@ArnyminerZ ArnyminerZ requested a review from rfc2822 June 25, 2024 10:00
ArnyminerZ and others added 10 commits June 25, 2024 14:54
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@rfc2822 rfc2822 force-pushed the 837-replace-appintro-by-compose-pager branch from 16cc3d0 to c7b18b1 Compare June 25, 2024 12:54
@rfc2822 rfc2822 merged commit e92f261 into main-ose Jun 25, 2024
5 of 6 checks passed
@rfc2822 rfc2822 deleted the 837-replace-appintro-by-compose-pager branch June 25, 2024 13:12
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.

Replace AppIntro by Compose Pager
4 participants