Fix Part of #4938: Redesign Profile login and PIN Reset(1/12) #5825
Conversation
# Conflicts: # app/src/main/res/values/styles.xml
# Conflicts: # app/src/main/res/values/color_palette.xml # third_party/maven_install.json
|
Thanks @deonwaju! I have addressed your comments. Please resolve if you have the permissions, otherwise let me know and I will resolve them myself. |
There was a problem hiding this comment.
Thanks @adhiamboperes! I haven't looked at the bulky bits of the PR yet (the new Composables, the new fragment presenter, and the fragment test), but did look at everything else and left some comments. Besides those, could you also fix CI & look at the replies below?
Other Design Choices
Dialog State Management:
- When opening two dialogs in sequence in compose, I chose to display the second dialog without dismissing the first one. This causes the second dialog to appear on top of the first. When the second dialog is dismissed, the first remains open and must be manually closed.
- On the flip side, if the first dialog is configured to close when the positive button is clicked(to open the next dialog), it dismisses correctly, but then the second dialog doesn’t appear at all.
Neither approach is ideal, but keeping the first dialog open while showing the second is the more acceptable of the two.
This shouldn't be necessary, I think. In the non-Compose world you can check if the fragment has already been added to the activity's fragment manager (via its tag) and replace it with a new one (or not open a new one at all) to de-duplicate the dialogs. Shouldn't there be a way to do the same via Compose?
Misc
There are no dark mode screenshots since the design team is still working on those(oppia/design-team#192), and we will need to include them in a cleanup PR.
Is there a tracking issue for this that includes the details for updating this & any other affected flows in particular?
Essential Checklist
...
Can you please fill this out?
| @@ -0,0 +1,12 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" | |||
There was a problem hiding this comment.
Are we ever adding anything to this layout dynamically? If not, can we just make the FrameLayout the root, instead? It helps reducing the layout nesting.
There was a problem hiding this comment.
Not really. I have flattened the layout.
| android:background="@color/component_color_shared_screen_primary_background_color" | ||
| android:gravity="center"> | ||
|
|
||
| <androidx.compose.ui.platform.ComposeView |
There was a problem hiding this comment.
Similar to my comment for the profile_login_activity layout, can the ComposeView possibly be the top-level one here? I'm assuming the gravity doesn't matter since the ComposeView has a match_parent width/height, and we should be able to set the background of the ComposeView (I'm assuming).
There was a problem hiding this comment.
Changed the top-level view to be the ComposeView
There was a problem hiding this comment.
For this & the other class: we need to start thinking about naming standards for these files since they're a new type of file altogether that we haven't really had before.
https://android.googlesource.com/platform/frameworks/support/+/androidx-main/compose/docs/compose-api-guidelines.md#naming-unit-composable-functions-as-entities explains this, but it's honestly quite a bit more complicated than I was expecting. I suspect we'll need to think about this and have a lot more examples before we can settle on naming conventions that make sense to us.
I don't think there's an action item here, but I did want to get your thoughts on the matter, if you have any.
There was a problem hiding this comment.
Generally, I believe that naming composable entities should follow the same conventions as the existing files in the project. We typically have FeatureNameActivity/Fragment/Dialog, and for custom views, we have CustomView. Composable entities will likely fall into 3 categories:
- the composable functions that represent small view units, and will be written inside a presenter(or a larger Composable class, e.g.
- composables that represent screens, standalone componets such as dialogs
- and finally, custom views, e.g e.g. AllClassroomsHeaderText.kt
The list is not exhaustive, but will form a bulk of Composables that we write.
Combining our present naming convention with the Noun imperative from the docs, it shouldn't be too complex.
ComingSoonTopicList.ktwould be renamed toComingSoonTopicListView.kt- This dialog can be renamed from
ConfirmDataResetDialog.kttoDataResetConfirmationDialog, - and the smaller unit composables can be named explicitly after the view they represent, e.g
NameErrorText()
|
|
||
| /** Handles showing the [ResetPinDialogFragment]. */ | ||
| fun handleRouteToResetPinDialog(profileId: ProfileId, profileName: String) { | ||
|
|
There was a problem hiding this comment.
Nit: please remove empty line for convention.
| as DialogFragment | ||
| resetPinDialog.dismiss() | ||
|
|
||
| fragmentManager.executePendingTransactions() |
There was a problem hiding this comment.
For here & above: why is this needed?
There was a problem hiding this comment.
Since there is a series of dialogs leading to one another, forcing all pending transactions to complete immediately helps prevent stacking and errors.
|
Unassigning @BenHenning since the review is done. |
|
Hi @adhiamboperes, it looks like some changes were requested on this pull request by @BenHenning. PTAL. Thanks! |
Compose Navigation replaces Fragment transactions and tags entirely, but has state. For hiding/showing Composables, we use
#4938 should track all the work related to this project.
Done |
Coverage ReportResultsNumber of files assessed: 19 Passing coverageFiles with passing code coverage
Exempted coverageFiles exempted from coverage
|
|
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. |
Explanation
Fixes Part of #4938
This PR introduces the following changes:
Detailed Explanation of the Changes
Use of Jetpack Compose
We agreed that we can use Jetpack Compose for new screens being introduced, complete with its own activity and fragment. Although there is an existing login screen, the UI and flows are a little different from the new one, so I opted to create an new screen. The legacy screen has TODOs added to earmark it for removal.
The PR introduces Compose UIs for the screen, as well as the admin pin reset dialogs, since these are alert dialogs. The non-admin pin reset dialog reuses the existing dialogs, since they are standalone dialog fragments.
The Composable functions are private because they are not being tested directly, like you would in in a purely Compose project. The two admin PIN reset dialogs are however in their own files because it made sense to isolate them at that level, being that they are independent components.
Finally, I added the
"androidx.compose.runtime:runtime-livedata": "1.1.1",dependency to be able to observe livedata objects from within composables.Test Design
Because the screen contains both Compose and non-compose components, some of the tests use both Compose nodes and Espresso Matchers and ViewInteractions. I am pleased with how well the dialogs tested.
I used
createEmptyComposeRule()as the test rule because I found out previously in #5387 that it is the most appropriate rule to use when there is need to launch a new activity for every test, which in our case is useful for testing with different feature flag states.maven_install.jsonDue to conflicts with develop, I had to regenerate this file since we can't directly edit it.
Misc
There are no dark mode screenshots since the design team is still working on those(oppia/design-team#192), and we will need to include them in a cleanup PR.
Essential Checklist
For UI-specific PRs only