UI Changes to Connect ID Device Configuration Workflow #3109
Conversation
…ragment ,updated new ccc logo
pm-dimagi
added
skip-integration-tests
📝 WalkthroughWalkthroughThis set of changes removes the ConnectID flow from the application and replaces it with a new PersonalID flow. Multiple XML layout files and Java fragment classes related to ConnectID phone number input, password verification, secondary phone input, OTP verification, and associated bottom sheets are deleted. The navigation graph is refactored: all ConnectID fragments and dialogs are renamed or removed, and the flow is streamlined under the PersonalID namespace. Several user-facing strings are updated to reflect the new terminology. New layouts and fragments are introduced for PersonalID, focusing on phone and name input, and backup code handling. Sequence Diagram(s)sequenceDiagram
participant User
participant PersonalIDPhoneFragment
participant PersonalIDOTPPage
participant PersonalIDNameFragment
participant PersonalIDBiometricConfigFragment
participant PersonalIDBackupCodeFragment
User->>PersonalIDPhoneFragment: Enter phone number
PersonalIDPhoneFragment->>PersonalIDOTPPage: Navigate on valid phone
User->>PersonalIDOTPPage: Enter OTP
PersonalIDOTPPage->>PersonalIDNameFragment: Navigate on valid OTP
User->>PersonalIDNameFragment: Enter name
PersonalIDNameFragment->>PersonalIDBiometricConfigFragment: Continue
PersonalIDBiometricConfigFragment->>PersonalIDBackupCodeFragment: Setup biometric, continue
PersonalIDBackupCodeFragment-->>User: Display backup code
Possibly related PRs
Suggested reviewers
Tip ⚡️ Free AI Code Reviews for VS Code, Cursor, Windsurf
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (12)
app/res/layout/screen_personalid_phone_verify.xml (1)
19-19: Ensure accessibility for decorative logo
The logo@drawable/ccc_logo_fullis decorative (android:contentDescription="@null"). Consider addingandroid:importantForAccessibility="no"to explicitly mark it as decorative, improving clarity for TalkBack.app/res/layout/fragment_recovery_code.xml (1)
18-18: Accessibility: mark decorative image
The company logo is now@drawable/ccc_logo_full. Since it’s decorative, in addition toandroid:contentDescription="@null", addandroid:importantForAccessibility="no".app/res/layout/screen_personalid_phoneno.xml (1)
30-30: Mark decorative logo for accessibility
Logo updated to@drawable/ccc_logo_full. Consider addingandroid:importantForAccessibility="no"for decorative images.app/res/layout/personalid_name_fragment.xml (2)
24-29: Accessibility: mark logo decorative
The logo image@drawable/ccc_logo_fullis decorative. To improve accessibility, addandroid:contentDescription="@null"(if not present) andandroid:importantForAccessibility="no".
75-85: Remove unnecessary LTR symmetry ignore
Suppressing RTL symmetry (tools:ignore="RtlSymmetry") here may not be needed if your layout inherently supports RTL. Revisit this decision or scope it more narrowly.app/res/values/strings.xml (4)
416-418: Consider renaming resource IDs for clarity
The stringsconnect_registration_titleandconnect_registration_subtitlenow contain “PersonalID” copy. For consistency with the PersonalID feature, consider renaming them (e.g.personalid_registration_title).
513-513: Inconsistent capitalization of 'PersonalId'
The string uses “PersonalId” (lowercase 'd') but elsewhere in the app “PersonalID” is capitalized. Update to “PersonalID” for consistency.
658-663: Hyphenate compound modifiers
Please adjust the following for proper typographic style:
- “One Time Password” → “One-time Password”
- “6 Digit Backup Code” → “6-digit Backup Code”
908-910: Prefix new string resources
The new stringsname_fragment_subtitleandname_empty_errorlack a prefix. To avoid name collisions and maintain consistency, consider renaming topersonalid_name_subtitleandpersonalid_name_empty_error.app/res/navigation/nav_graph_connectid.xml (3)
5-6: Rename navigation graph ID to reflect PersonalID
The navigation graph ID (@+id/nav_graph_connectid) and filename still reference “ConnectID”, which can cause confusion when wiring this graph elsewhere. Consider renaming the ID (and the file) to something like@+id/nav_graph_personalidto clearly convey the new PersonalID flow.
181-188: Clean up whitespace and standardizepersonalid_namelabel
There are extra blank lines before thepersonalid_namefragment. Additionally, the labelpersonalid_namecould follow thefragment_*convention (e.g.,fragment_personalid_name) for consistency with other fragments.
9-10: Consider moving fragments to a PersonalID package
All fragments are still underorg.commcare.fragments.connectIddespite being part of the PersonalID flow. For clarity and maintainability, you may want to refactor these classes into aorg.commcare.fragments.personalIdpackage to separate legacy ConnectID components from the new implementation.Also applies to: 28-29, 71-72, 122-123, 157-158, 181-182
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
app/res/drawable-ldpi/ccc_logo_full.pngis excluded by!**/*.pngapp/res/drawable-mdpi/ccc_logo_full.pngis excluded by!**/*.pngapp/res/drawable-xhdpi/ccc_logo_full.pngis excluded by!**/*.pngapp/res/drawable-xxhdpi/ccc_logo_full.pngis excluded by!**/*.pngapp/res/drawable-xxxhdpi/ccc_logo_full.pngis excluded by!**/*.png
📒 Files selected for processing (16)
app/res/layout/fragment_phone_available_bottom_sheet.xml(0 hunks)app/res/layout/fragment_recovery_code.xml(1 hunks)app/res/layout/fragment_secondary_phone_number.xml(0 hunks)app/res/layout/personalid_name_fragment.xml(1 hunks)app/res/layout/screen_connect_password_verify.xml(0 hunks)app/res/layout/screen_connect_primary_phone.xml(0 hunks)app/res/layout/screen_connect_user_deactivate_otp_verify.xml(0 hunks)app/res/layout/screen_personalid_phone_verify.xml(1 hunks)app/res/layout/screen_personalid_phoneno.xml(3 hunks)app/res/navigation/nav_graph_connectid.xml(5 hunks)app/res/values/strings.xml(4 hunks)app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java(0 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java(0 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPhoneAvailableBottomSheet.java(0 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPhoneFragment.java(0 hunks)app/src/org/commcare/fragments/connectId/ConnectIdUserDeactivateOTPVerificationFragment.java(0 hunks)
💤 Files with no reviewable changes (10)
- app/src/org/commcare/fragments/connectId/ConnectIdPhoneAvailableBottomSheet.java
- app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java
- app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java
- app/res/layout/screen_connect_user_deactivate_otp_verify.xml
- app/res/layout/fragment_phone_available_bottom_sheet.xml
- app/res/layout/screen_connect_primary_phone.xml
- app/src/org/commcare/fragments/connectId/ConnectIdPhoneFragment.java
- app/res/layout/fragment_secondary_phone_number.xml
- app/res/layout/screen_connect_password_verify.xml
- app/src/org/commcare/fragments/connectId/ConnectIdUserDeactivateOTPVerificationFragment.java
🧰 Additional context used
🧠 Learnings (1)
app/res/navigation/nav_graph_connectid.xml (1)
Learnt from: Jignesh-dimagi
PR: dimagi/commcare-android#3093
File: app/res/navigation/nav_graph_connect_messaging.xml:41-45
Timestamp: 2025-05-09T10:57:41.073Z
Learning: In the CommCare Android codebase, the navigation graph for Connect messaging (`nav_graph_connect_messaging.xml`) intentionally uses `channel_id` as the argument name in the connectMessageFragment, despite using `channelId` in other parts of the same navigation graph. This naming difference is by design in the refactored code.
🔇 Additional comments (4)
app/res/layout/screen_personalid_phoneno.xml (2)
9-9: Verify preview context path
tools:context="org.commcare.fragments.connectId.PersonalIdPhoneFragment"may no longer match the actual package/class. Update this to the correctPersonalIdPhoneFragmentpath (likely under apersonalidpackage).
48-48: Confirm updated subtitle resource
Using@string/connect_registration_subtitleis correct, but note theconnect_prefix remains. Ensure this resource name aligns with the PersonalID flow, or consider renaming for consistency.app/res/navigation/nav_graph_connectid.xml (2)
21-23: Verify the renamed biometric-to-OTP navigation action
The newly addedaction_personalid_biometric_config_to_personalid_otp_pagelooks correct, but please confirm that the destination ID@id/personalid_otp_pagematches the target fragment’s ID in this graph.
62-67: Confirm new OTP fragment actions
The new actionsaction_personalid_otp_page_to_personalid_nameandaction_personalid_otp_page_to_personalid_phone_fragmentare correctly named. Verify that any code or tests referencing the old actions have been updated accordingly.
| <TextView | ||
| android:id="@+id/name_fragment_subtitle" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:layout_marginStart="16dp" | ||
| android:textStyle="bold" | ||
| android:layout_marginTop="16dp" | ||
| android:layout_marginEnd="16dp" | ||
| android:text="@string/name_fragment_subtitle" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Keyboard type for name input
The TextInputEditText for name entry lacks an explicit android:inputType. Add android:inputType="textPersonName" to bring up the name-appropriate IME keyboard.
🤖 Prompt for AI Agents (early access)
In app/res/layout/personalid_name_fragment.xml around lines 39 to 47, the
TextInputEditText used for name input is missing the android:inputType
attribute. Add android:inputType="textPersonName" to this TextInputEditText
element to ensure the appropriate keyboard is shown for name entry.
| android:layout_width="match_parent" | ||
| android:layout_height="match_parent" | ||
| android:background="@color/white" | ||
| tools:context="org.commcare.fragments.connectId.PersonalIdPhoneFragment"> |
There was a problem hiding this comment.
Correct preview context for NameFragment
tools:context="org.commcare.fragments.connectId.PersonalIdPhoneFragment" is incorrect for this layout. It should reference PersonalIdNameFragment (e.g., org.commcare.fragments.personalid.PersonalIdNameFragment).
🤖 Prompt for AI Agents (early access)
In app/res/layout/personalid_name_fragment.xml at line 9, the tools:context
attribute incorrectly references PersonalIdPhoneFragment. Update this attribute
to point to the correct fragment class, PersonalIdNameFragment, using the
appropriate package path such as
org.commcare.fragments.personalid.PersonalIdNameFragment to ensure the preview
context matches the layout.
| <TextView | ||
| android:textStyle="bold" | ||
| android:id="@+id/errorTextView" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:layout_marginStart="84dp" | ||
| android:paddingTop="4dp" | ||
| android:textColor="@android:color/holo_red_light" | ||
| android:textSize="14sp" | ||
| android:visibility="gone" | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid generic errorTextView ID
The ID errorTextView is reused in multiple layouts, which can lead to collisions when using view binding or findViewById. Rename this to a more specific name (e.g. personalid_name_error) for clarity.
🤖 Prompt for AI Agents (early access)
In app/res/layout/personalid_name_fragment.xml around lines 89 to 99, the
TextView uses a generic ID "errorTextView" which is reused in multiple layouts
causing potential collisions. Rename the ID to a more specific and unique name
like "personalid_name_error" to avoid conflicts and improve clarity when using
view binding or findViewById.
| <com.google.android.material.button.MaterialButton | ||
| android:id="@+id/continue_button" | ||
| style="@style/CustomButtonStyle" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:layout_gravity="end" | ||
| android:layout_marginTop="16dp" | ||
| android:layout_marginEnd="16dp" | ||
| android:gravity="start|center" | ||
| android:text="@string/review" | ||
| android:drawableRight="@drawable/connect_right_arrow" | ||
| /> | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unique ID for continue button
@+id/continue_button is generic and duplicated elsewhere. Consider using a more descriptive ID (e.g. personalid_name_continue_button) to avoid confusion and improve maintainability.
🤖 Prompt for AI Agents (early access)
In app/res/layout/personalid_name_fragment.xml around lines 100 to 112, the
continue button uses a generic ID "@+id/continue_button" which may be duplicated
elsewhere. Rename this ID to a more descriptive and unique one like
"personalid_name_continue_button" to prevent conflicts and improve code clarity.
| android:id="@+id/personalid_otp_page" | ||
| android:name="org.commcare.fragments.connectId.PersonalIdPhoneVerificationFragment" | ||
| android:label="fragment_connectid_phone_verify" | ||
| tools:layout="@layout/screen_connect_phone_verify"> | ||
| <action | ||
| android:id="@+id/action_connectid_phone_verify_to_connectid_pin" | ||
| app:destination="@id/connectid_pin" | ||
| app:popUpTo="@id/connectid_phone_verify" /> | ||
| <action | ||
| android:id="@+id/action_connectid_phone_verify_to_connectid_biometric_config" | ||
| app:destination="@id/connectid_biometric_config" | ||
| app:popUpTo="@id/connectid_phone_verify" /> | ||
| <action | ||
| android:id="@+id/action_connectid_phone_verify_to_connectid_password" | ||
| app:destination="@id/connectid_password" | ||
| app:popUpTo="@id/connectid_phone_verify" /> | ||
| tools:layout="@layout/screen_personalid_phone_verify"> | ||
| <action | ||
| android:id="@+id/action_connectid_phone_verify_to_connectid_message" | ||
| app:destination="@id/connectid_message" | ||
| app:popUpTo="@id/connectid_phone_verify" /> | ||
| <action | ||
| android:id="@+id/action_connectid_phone_verify_self" | ||
| app:destination="@id/connectid_phone_verify" | ||
| app:popUpTo="@id/connectid_phone_verify" /> | ||
| <action | ||
| android:id="@+id/action_connectid_phone_verify_to_connectid_signup_fragment" | ||
| app:destination="@id/connectid_signup_fragment" /> | ||
| app:destination="@id/personalid_message_display" | ||
| app:popUpTo="@id/personalid_otp_page" /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align OTP fragment label and first action ID
The android:label is still fragment_connectid_phone_verify, and the first action ID is action_connectid_phone_verify_to_connectid_message. Both should be updated to the PersonalID naming (e.g., fragment_personalid_otp_page and action_personalid_otp_page_to_personalid_message).
🤖 Prompt for AI Agents (early access)
In app/res/navigation/nav_graph_connectid.xml around lines 27 to 34, update the
android:label from "fragment_connectid_phone_verify" to
"fragment_personalid_otp_page" and rename the first action ID from
"action_connectid_phone_verify_to_connectid_message" to
"action_personalid_otp_page_to_personalid_message" to align with the PersonalID
naming convention.
| <fragment | ||
| android:id="@+id/connectid_signup_fragment" | ||
| android:name="org.commcare.fragments.connectId.ConnectIDSignupFragment" | ||
| android:label="fragment_connectid_signup_fragment" | ||
| tools:layout="@layout/fragment_signup"> | ||
| android:id="@+id/personalid_phone_fragment" | ||
| android:name="org.commcare.fragments.connectId.PersonalIdPhoneFragment" | ||
| android:label="fragment_personalid_phone_fragment" | ||
| tools:layout="@layout/screen_personalid_phoneno"> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update action ID in phone-entry fragment
The first action in personalid_phone_fragment is still named action_connectid_phone_fragment_to_connectid_message. Rename it to something like action_personalid_phone_fragment_to_personalid_message_display to align with the rest of the graph.
Also applies to: 171-175
🤖 Prompt for AI Agents (early access)
In app/res/navigation/nav_graph_connectid.xml around lines 156 to 160 and 171 to
175, the action ID in the personalid_phone_fragment is incorrectly named as
action_connectid_phone_fragment_to_connectid_message. Rename this action ID to
action_personalid_phone_fragment_to_personalid_message_display to maintain
consistent naming conventions across the navigation graph.
| <fragment | ||
| android:id="@+id/personalid_message_display" | ||
| android:name="org.commcare.fragments.connectId.ConnectIdMessageFragment" | ||
| android:label="fragment_connectid_message" | ||
| tools:layout="@layout/screen_connect_message"> | ||
| tools:layout="@layout/screen_personalid_message"> | ||
| <action | ||
| android:id="@+id/action_connectid_message_to_connectid_biometric_config" | ||
| app:destination="@id/connectid_biometric_config" | ||
| app:popUpTo="@id/connectid_message" /> | ||
| app:destination="@id/personalid_biometric_config" | ||
| app:popUpTo="@id/personalid_message_display" /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor personalid_message_display IDs and labels
Within the PersonalID message fragment, both the android:label (fragment_connectid_message) and several action IDs (action_connectid_message_to_connectid_biometric_config, action_connectid_message_to_connectid_pin, action_connectid_message_to_connectid_phone_verify) still use ConnectID prefixes. Rename these to fragment_personalid_message_display, action_personalid_message_to_personalid_biometric_config, etc., to avoid mixing flows.
Also applies to: 80-87
🤖 Prompt for AI Agents (early access)
In app/res/navigation/nav_graph_connectid.xml around lines 70 to 87, the
fragment and action IDs and labels use ConnectID prefixes instead of PersonalID.
Rename the fragment's android:label to "fragment_personalid_message_display" and
update all related action IDs by replacing "connectid" with "personalid" and
adjusting the names accordingly, such as changing
"action_connectid_message_to_connectid_biometric_config" to
"action_personalid_message_to_personalid_biometric_config" to maintain
consistent naming and avoid mixing flows.
| <fragment | ||
| android:id="@+id/connectid_biometric_config" | ||
| android:name="org.commcare.fragments.connectId.ConnectIdBiometricConfigFragment" | ||
| android:id="@+id/personalid_biometric_config" | ||
| android:name="org.commcare.fragments.connectId.PersonalIdBiometricConfigFragment" | ||
| android:label="fragment_connectid_biometric_config" | ||
| tools:layout="@layout/screen_connect_verify"> | ||
| <action | ||
| android:id="@+id/action_connectid_biometric_config_to_connectid_phone_verify" | ||
| app:destination="@id/connectid_phone_verify" | ||
| app:popUpTo="@id/connectid_biometric_config" /> | ||
| tools:layout="@layout/screen_personalid_verify"> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update labels and action IDs for personalid_biometric_config
The android:label (fragment_connectid_biometric_config) and action ID (action_connectid_biometric_config_to_connectid_message) still use the old ConnectID prefixes. Rename them to e.g. fragment_personalid_biometric_config and action_personalid_biometric_config_to_personalid_message for consistency.
Also applies to: 14-16
🤖 Prompt for AI Agents (early access)
In app/res/navigation/nav_graph_connectid.xml around lines 8 to 12 and 14 to 16,
update the android:label attribute from "fragment_connectid_biometric_config" to
"fragment_personalid_biometric_config" and rename the action ID from
"action_connectid_biometric_config_to_connectid_message" to
"action_personalid_biometric_config_to_personalid_message" to maintain
consistent naming conventions. Make sure to apply these changes to both the
fragment label and the corresponding action IDs.
| android:id="@+id/personalid_backup_code" | ||
| android:name="org.commcare.fragments.connectId.PersonalIdPinFragment" | ||
| android:label="fragment_connectid_pin" | ||
| tools:layout="@layout/fragment_recovery_code"> | ||
| <action | ||
| android:id="@+id/action_connectid_pin_to_connectid_phone_verify" | ||
| app:destination="@id/connectid_phone_verify" | ||
| app:popUpTo="@id/connectid_pin" /> | ||
| <action | ||
| android:id="@+id/action_connectid_pin_to_connectid_message" | ||
| app:destination="@id/connectid_message" | ||
| app:popUpTo="@id/connectid_pin" /> | ||
| app:destination="@id/personalid_message_display" | ||
| app:popUpTo="@id/personalid_backup_code" /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Correct labels and action IDs in backup-code fragment
The android:label remains fragment_connectid_pin, and actions are action_connectid_pin_to_connectid_message / action_connectid_pin_self. Rename to fragment_personalid_backup_code and action_personalid_backup_code_to_personalid_message, action_personalid_backup_code_self for consistency.
Also applies to: 131-133
🤖 Prompt for AI Agents (early access)
In app/res/navigation/nav_graph_connectid.xml around lines 122 to 129 and also
lines 131 to 133, update the android:label from "fragment_connectid_pin" to
"fragment_personalid_backup_code" and rename the action IDs from
"action_connectid_pin_to_connectid_message" and "action_connectid_pin_self" to
"action_personalid_backup_code_to_personalid_message" and
"action_personalid_backup_code_self" respectively to maintain consistent naming
conventions.
shubham1g5
changed the title
|
@pm-dimagi Can you create a separate PR for naming changes and PR it separately and then target this branch to the new name change PR. I want to mimize diff in the PR here and keep it to what it actually is. |
|
@pm-dimagi Can we use Kotlin for any new code? |
|
@coderabbitai resolved issue here ad96e31 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 65602 @pm-dimagi I see you've committed the naming convention changes in ad96e31. Thanks for addressing that feedback! The changes from ConnectID to PersonalID look good. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
…nto pm_CCT_1136 # Conflicts: # app/res/layout/screen_connect_user_deactivate_otp_verify.xml # app/res/navigation/nav_graph_connectid.xml
That's something we want to do as a team but we should discuss together on when is the right time to do it, we will need some learning time for the team to do this transition smoothly without affecting project deadlines. IMO, we should see off the merge work first and afterwards plan one code improvement initiative at a time as we likely will have more breathing space then. |
|
@shubham1g5 Agree. |
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
I think we should try to get the name change from ConnectID to PersonalID done in one go, so suggested a few other places where I see renaming to be done (but there are others).
Also note the CodeRabbit comments, at least some of which seem appropriate to address.
app/src/org/commcare/fragments/connectId/PersonalIdBiometricConfigFragment.java
Show resolved
Hide resolved
app/src/org/commcare/fragments/connectId/PersonalIdBackupCodeFragment.java
Outdated
Show resolved
Hide resolved
…nto pm_CCT_1136 # Conflicts: # app/res/navigation/nav_graph_connectid.xml # app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java # app/src/org/commcare/fragments/connectId/ConnectIdPhoneFragment.java # app/src/org/commcare/fragments/connectId/PersonalIdBackupCodeFragment.java # app/src/org/commcare/fragments/connectId/PersonalIdBiometricConfigFragment.java # app/src/org/commcare/fragments/connectId/PersonalIdPasswordVerificationFragment.java # app/src/org/commcare/fragments/connectId/PersonalIdPhoneFragment.java
app/src/org/commcare/fragments/connectId/PersonalIdNameFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/connectId/PersonalIdNameFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/connectId/PersonalIdNameFragment.java
Outdated
Show resolved
Hide resolved
| <layout xmlns:android="http://schemas.android.com/apk/res/android"> | ||
|
|
||
| <androidx.constraintlayout.widget.ConstraintLayout xmlns:app="http://schemas.android.com/apk/res-auto" |
There was a problem hiding this comment.
@pm-dimagi Please remove these 2, we can directly starts with scroll view as parent. It will reduce layout nesting
| <ScrollView | ||
| android:id="@+id/firstLayout" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="0dp" |
There was a problem hiding this comment.
@pm-dimagi Why is height 0dp? It should be match_parent
|
|
||
| <View | ||
| android:id="@+id/divider" | ||
| android:layout_width="fill_parent" |
There was a problem hiding this comment.
@pm-dimagi fill_parent is depreciated, use match_parent
| <ImageView | ||
| android:id="@+id/company_logo" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="40dp" |
There was a problem hiding this comment.
@pm-dimagi Its not good to give specific height to image view, as logo size can alter depending upon the device density. Good to have wrap_content in height too.
| android:layout_width="42dp" | ||
| android:layout_height="42dp" |
There was a problem hiding this comment.
@pm-dimagi Parent node should have wrap_content in height and width.
| // Inflate the layout for this fragment | ||
| binding = PersonalidNameFragmentBinding.inflate(inflater, container, false); | ||
| activity = requireActivity(); | ||
| View view = binding.getRoot(); |
There was a problem hiding this comment.
@pm-dimagi Remove this variable view, return directly binding.getRoot()
| Bundle savedInstanceState) { | ||
| // Inflate the layout for this fragment | ||
| binding = PersonalidNameFragmentBinding.inflate(inflater, container, false); | ||
| activity = requireActivity(); |
There was a problem hiding this comment.
@pm-dimagi No need for activity variable, we can always use requireActivity() whenever need reference to activity
There was a problem hiding this comment.
using requireActivity() again and again to simplify reading used activity
|
|
||
|
|
||
| void updateUi(String errorMessage) { | ||
| if (errorMessage == null || errorMessage.isEmpty()) { |
There was a problem hiding this comment.
@pm-dimagi you can use TextUtils.isEmpty(errorMessage) which checks for null and empty char
| public View onCreateView(@NonNull LayoutInflater inflater, ViewGroup container, | ||
| Bundle savedInstanceState) { | ||
| // Inflate the layout for this fragment | ||
| binding = PersonalidNameFragmentBinding.inflate(inflater, container, false); |
There was a problem hiding this comment.
@pm-dimagi Cannot find xml file with name `personalid_name_fragment.xml' in this commit.
There was a problem hiding this comment.
@pm-dimagi It has file name screen_personalid_name
Then it should
binding = ScreenPersonalidName.inflate(inflater, container, false);
There was a problem hiding this comment.
| } | ||
|
|
||
|
|
||
| void updateUi(String errorMessage) { |
There was a problem hiding this comment.
@pm-dimagi When is this method called? May be you need to listen edit text change listener to remove the error when user is typing?
There was a problem hiding this comment.
this will be used after the api call is made
There was a problem hiding this comment.
Ok. Ideally, error should go away when user starts typing?
| @@ -0,0 +1,113 @@ | |||
| <layout xmlns:android="http://schemas.android.com/apk/res/android" | |||
There was a problem hiding this comment.
@pm-dimagi Can we start with ScrollView only
<?xml version="1.0" encoding="utf-8"?>
<ScrollView xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:app="http://schemas.android.com/apk/res-auto"
xmlns:tools="http://schemas.android.com/tools"
android:layout_width="match_parent"
android:layout_height="match_parent">
There was a problem hiding this comment.
this is necessary for nav graph implementation
There was a problem hiding this comment.
I don't think so.
you can check
and https://github.com/dimagi/commcare-android/blob/master/app/res/layout/fragment_channel_list.xmlIt doesn't have <layout..
There was a problem hiding this comment.
sorry we need layout for databinding
| tools:context="org.commcare.fragments.connectId.PersonalIdPhoneFragment"> | ||
|
|
||
| <ScrollView | ||
| <layout xmlns:android="http://schemas.android.com/apk/res/android" |
There was a problem hiding this comment.
@pm-dimagi Start with Scroll view
<?xml version="1.0" encoding="utf-8"?>
<ScrollView xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:app="http://schemas.android.com/apk/res-auto"
xmlns:tools="http://schemas.android.com/tools"
android:layout_width="match_parent"
android:layout_height="match_parent">
There was a problem hiding this comment.
this is necessary for nav graph implementation
There was a problem hiding this comment.
sorry we need layout for databinding
|
@damagatchi retest this please |
|
@damagatchi retest this please |
1 similar comment
|
@damagatchi retest this please |
|
@damagatchi retest this please |
Product Description
here is the design changes for the new upcoming journey of PersonalID device configration
Update 1
https://dimagi.atlassian.net/browse/CCCT-1141
https://dimagi.atlassian.net/browse/CCCT-1137
https://dimagi.atlassian.net/browse/CCCT-1136
https://dimagi.atlassian.net/browse/CCCT-1138
https://dimagi.atlassian.net/browse/CCCT-1140
These are the refactored design and the deletion of non used classes in upcoming journey
This code is completed now
Technical Summary
Feature Flag
New UI Journey
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review