-
Notifications
You must be signed in to change notification settings - Fork 500
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
Fix Part of #4938: Introduce Onboarding profile type screen [Blocked: #5373] #5378
base: new-onboarding-screen
Are you sure you want to change the base?
Fix Part of #4938: Introduce Onboarding profile type screen [Blocked: #5373] #5378
Conversation
…boarding-profile-type-screen
…boarding-profile-type-screen # Conflicts: # app/src/main/res/values/component_colors.xml # scripts/assets/test_file_exemptions.textproto
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.
Self- review pass
app/src/main/java/org/oppia/android/app/onboardingv2/OnboardingProfileTypeFragmentPresenter.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/onboardingv2/OnboardingFragmentPresenter.kt
Outdated
Show resolved
Hide resolved
…d into onboarding-profile-type-screen
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.
Thanks @adhiamboperes! Had a few comments--PTAL.
Also per my review of the blocking PR, I wonder if we could just keep everything under onboarding/ rather than having a separate package. WDYT?
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.
For this & the other PNG--can these also be SVGs, instead?
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.
For these specific images, we do not have svg versions.
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.
I wonder if they have source materials (e.g. Illustrator) that can be used to export SVG versions. The concern with using PNGs (aside from the binary implications in the repository) is that they won't scale correctly for different display resolutions without introducing multiple copies of the PNGs. SVGs scale arbitrarily and should always look correct.
app/src/sharedTest/java/org/oppia/android/app/onboarding/OnboardingProfileTypeFragmentTest.kt
Outdated
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/android/app/onboarding/OnboardingProfileTypeFragmentTest.kt
Show resolved
Hide resolved
Hi, @adhiamboperes, the LGTM Label has been removed because the changes were requested on this PR. Thanks!. |
Hi @adhiamboperes, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
…boarding-profile-type-screen # Conflicts: # app/src/main/java/org/oppia/android/app/activity/ActivityComponentImpl.kt # app/src/main/java/org/oppia/android/app/onboarding/OnboardingFragment.kt # app/src/main/java/org/oppia/android/app/onboarding/OnboardingFragmentPresenter.kt # scripts/assets/test_file_exemptions.textproto
@BenHenning, PTAL. |
Hi @adhiamboperes, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
APK & AAB differences analysisNote that this is a summarized snapshot. See the CI artifacts for detailed differences. DevExpand to see flavor specificsUniversal APKAPK file size: 17 MiB (old), 17 MiB (new), 96 KiB (Added) APK download size (estimated): 16 MiB (old), 16 MiB (new), 90 KiB (Added) Method count: 235368 (old), 235511 (new), 143 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6550 (old), 6587 (new), 37 (Added)
Lesson assets: 105 (old), 105 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 17 MiB (old), 17 MiB (new), 95 KiB (Added)
Configuration hdpiAPK file size: 59 KiB (old), 59 KiB (new), 28 bytes (Added) Configuration ldpiAPK file size: 56 KiB (old), 56 KiB (new), 36 bytes (Added) Configuration mdpiAPK file size: 53 KiB (old), 53 KiB (new), 32 bytes (Added) Configuration tvdpiAPK file size: 103 KiB (old), 103 KiB (new), 48 bytes (Added) Configuration xhdpiAPK file size: 67 KiB (old), 67 KiB (new), 32 bytes (Added) Configuration xxhdpiAPK file size: 76 KiB (old), 76 KiB (new), 32 bytes (Added) Configuration xxxhdpiAPK file size: 79 KiB (old), 79 KiB (new), 36 bytes (Added) AlphaExpand to see flavor specificsUniversal APKAPK file size: 9 MiB (old), 9 MiB (new), 91 KiB (Added) APK download size (estimated): 9044 KiB (old), 9130 KiB (new), 85 KiB (Added) Method count: 93608 (old), 93698 (new), 90 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5504 (old), 5540 (new), 36 (Added)
Lesson assets: 105 (old), 105 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 9 MiB (old), 9 MiB (new), 90 KiB (Added)
Configuration hdpiAPK file size: 52 KiB (old), 52 KiB (new), 12 bytes (Added) Configuration ldpiAPK file size: 52 KiB (old), 52 KiB (new), 20 bytes (Added) Configuration mdpiAPK file size: 46 KiB (old), 46 KiB (new), 12 bytes (Added) Configuration tvdpiAPK file size: 90 KiB (old), 90 KiB (new), 20 bytes (Added) Configuration xhdpiAPK file size: 60 KiB (old), 60 KiB (new), 16 bytes (Added) Configuration xxhdpiAPK file size: 69 KiB (old), 69 KiB (new), 12 bytes (Added) Configuration xxxhdpiAPK file size: 71 KiB (old), 71 KiB (new), 12 bytes (Added) BetaExpand to see flavor specificsUniversal APKAPK file size: 9 MiB (old), 9 MiB (new), 91 KiB (Added) APK download size (estimated): 9032 KiB (old), 9116 KiB (new), 83 KiB (Added) Method count: 93608 (old), 93698 (new), 90 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5504 (old), 5540 (new), 36 (Added)
Lesson assets: 105 (old), 105 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 9 MiB (old), 9 MiB (new), 90 KiB (Added)
Configuration hdpiAPK file size: 52 KiB (old), 52 KiB (new), 12 bytes (Added) Configuration ldpiAPK file size: 52 KiB (old), 52 KiB (new), 20 bytes (Added) Configuration mdpiAPK file size: 46 KiB (old), 46 KiB (new), 12 bytes (Added) Configuration tvdpiAPK file size: 90 KiB (old), 90 KiB (new), 20 bytes (Added) Configuration xhdpiAPK file size: 60 KiB (old), 60 KiB (new), 16 bytes (Added) Configuration xxhdpiAPK file size: 69 KiB (old), 69 KiB (new), 12 bytes (Added) Configuration xxxhdpiAPK file size: 71 KiB (old), 71 KiB (new), 12 bytes (Added) GaExpand to see flavor specificsUniversal APKAPK file size: 9 MiB (old), 9 MiB (new), 90 KiB (Added) APK download size (estimated): 9032 KiB (old), 9116 KiB (new), 84 KiB (Added) Method count: 93608 (old), 93698 (new), 90 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5504 (old), 5540 (new), 36 (Added)
Lesson assets: 105 (old), 105 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 9 MiB (old), 9 MiB (new), 90 KiB (Added)
Configuration hdpiAPK file size: 52 KiB (old), 52 KiB (new), 12 bytes (Added) Configuration ldpiAPK file size: 52 KiB (old), 52 KiB (new), 20 bytes (Added) Configuration mdpiAPK file size: 46 KiB (old), 46 KiB (new), 12 bytes (Added) Configuration tvdpiAPK file size: 90 KiB (old), 90 KiB (new), 20 bytes (Added) Configuration xhdpiAPK file size: 60 KiB (old), 60 KiB (new), 16 bytes (Added) Configuration xxhdpiAPK file size: 69 KiB (old), 69 KiB (new), 12 bytes (Added) Configuration xxxhdpiAPK file size: 71 KiB (old), 71 KiB (new), 12 bytes (Added) |
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.
Thanks @adhiamboperes! Mostly LGTM, but had a few comments--PTAL.
app:customBackgroundColor="@{@color/component_color_onboarding_profile_type_background_color}" | ||
app:layout_constraintTop_toBottomOf="@id/profile_type_center_guide" /> | ||
|
||
<androidx.constraintlayout.widget.ConstraintLayout |
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.
Can this be flattened? I think most (all?) nested ConstraintLayouts should theoretically be simplifiable (except when separated by a non-constraint layout like MaterialCardView below).
} | ||
} | ||
|
||
@RunOn(TestPlatform.ESPRESSO) // Testing lifecycle fails on Robolectric. |
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.
Alternatively, you could verify that the activity is finishing using Activity.isFinishing() which I think is Robolectric-friendly.
app/src/sharedTest/java/org/oppia/android/app/onboarding/OnboardingProfileTypeFragmentTest.kt
Show resolved
Hide resolved
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.
I wonder if they have source materials (e.g. Illustrator) that can be used to export SVG versions. The concern with using PNGs (aside from the binary implications in the repository) is that they won't scale correctly for different display resolutions without introducing multiple copies of the PNGs. SVGs scale arbitrarily and should always look correct.
Explanation
Fixes Part of #4938: Introduce Onboarding profile type screen
Adds the profile type selection screen.
The new "I am a student" flow is not implemented, and tests have been set up to fail when the flow has been implemented.
The "I am a parent/teacher" flow has been set up to route to the existing profile screen.
Essential Checklist
All tests pass on Espresso
![Screenshot 2024-05-23 at 18 06 46](https://private-user-images.githubusercontent.com/59600948/333251535-6d2414c9-6ddf-4340-a408-52a7a5c8acdc.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTg2NTMyMTMsIm5iZiI6MTcxODY1MjkxMywicGF0aCI6Ii81OTYwMDk0OC8zMzMyNTE1MzUtNmQyNDE0YzktNmRkZi00MzQwLWE0MDgtNTJhN2E1YzhhY2RjLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA2MTclMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNjE3VDE5MzUxM1omWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTNlOGMzMWFlNTM1N2ZhYzhhNjFlYzI5ZGIyNTVmMzg5MjM0Y2UyNzk2NGRjZjk4Nzg3MThmMjA4YzdkZmYwNWImWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.r5sjkhCntsh_n1OLz0B9Ye9K5vt_4p_9bZTXWMb9xNA)
For UI-specific PRs only