Solved smaller screens UI issues #2980
Conversation
📝 WalkthroughWalkthroughThis pull request involves three main areas of change. First, two XML layout files have been refactored; one replaces a Sequence Diagram(s)sequenceDiagram
participant User
participant Activity
participant Manager as ConnectManager/ConnectIdBiometricConfigFragment
participant Helper as BiometricsHelper
participant BP as BiometricPrompt
User->>Activity: Initiate authentication
Activity->>Manager: Trigger authentication flow
Manager->>Helper: Call authenticateFingerprint(activity, biometricManager, callback)
Helper->>BP: Create unified authentication prompt (biometrics/device credentials)
BP-->>Helper: Return authentication result
Helper-->>Manager: Pass result back
Manager-->>Activity: Notify authentication result
Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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:
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: 0
🧹 Nitpick comments (5)
app/res/layout/fragment_signup.xml (5)
27-33: ScrollView Constraint Configuration:
Setting theScrollViewheight to0dpand constraining its top to@+id/llTopBarand bottom to@+id/ll_recoveris a smart approach for dynamic resizing. One important aspect to confirm is that the content within the ScrollView correctly measures its height and scrolls as expected.
35-39: ScrollView Child Layout Sizing:
The inner LinearLayout is set tolayout_height="match_parent", which may inhibit proper scrolling if the content overflows. It is generally advisable for a ScrollView’s direct child to usewrap_contentfor height. Consider switching towrap_contentto ensure the content sizes naturally based on its content.
49-56: Divider View Setup:
The divider view is effectively styled with a 1dp height and horizontal margins to create clear separation between sections. Note that the attributeandroid:layout_width="fill_parent"is used; consider updating this tomatch_parentto align with modern Android conventions.
81-117: Name Input Section Enhancement:
The restructuring of the name input area by combining an icon container and aTextInputEditTextimproves the visual grouping and likely the alignment of elements. One minor suggestion: theTextInputEditTextusesandroid:layout_marginLeft="16dp"(line 109). For better RTL (right-to-left) support, consider usingandroid:layout_marginStartinstead.
119-194: Phone Input Section Restructuring:
The phone input area is now more clearly divided: a left section for the phone icon and a right section for phone number inputs, which includes the country code and the primary phone input field along with an error text view beneath. This modular approach should aid small screen usability.
A small nitpick: in the nested LinearLayout (line 149),android:layout_marginLeftis used. Switching toandroid:layout_marginStartwould provide better support for different language directions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/res/layout/fragment_connect_delivery_details.xml(2 hunks)app/res/layout/fragment_signup.xml(2 hunks)app/src/org/commcare/connect/ConnectManager.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java(1 hunks)app/src/org/commcare/utils/BiometricsHelper.java(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java (2)
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java:95-131
Timestamp: 2025-02-04T21:38:11.970Z
Learning: Biometric authentication security improvements were considered but skipped in PR #2949 as per user's request. The implementation remained with basic biometric authentication without additional security layers.
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java:235-236
Timestamp: 2025-02-04T21:29:29.594Z
Learning: The empty performPasswordUnlock method in ConnectIdBiometricConfigFragment is intentionally left empty and should not be flagged in reviews.
🔇 Additional comments (15)
app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java (1)
237-239: Simplification of biometric authentication workflowThe code has been refactored to remove the
allowOtherOptionsparameter from the fingerprint authentication method call, which aligns with the updated method signature in BiometricsHelper. This change streamlines the authentication process and improves maintenance by centralizing authentication logic.app/src/org/commcare/connect/ConnectManager.java (1)
249-250: Consistent authentication method simplificationThe removal of the
allowOtherOptionsparameter from the fingerprint authentication method call matches the changes made in BiometricsHelper, maintaining consistency across the codebase. This simplification is part of the unified biometric authentication approach.app/res/layout/fragment_connect_delivery_details.xml (1)
158-190: Improved layout for smaller screens with ConstraintLayoutReplacing LinearLayout with ConstraintLayout provides better flexibility and control for accommodating different screen sizes. The specific changes include:
- Using
0dpwidth with constraints instead ofwrap_content- Adding proper constraint relationships between elements
- Adding a small right margin to improve spacing
These changes will help the UI display properly on smaller screens as per the PR objectives.
app/src/org/commcare/utils/BiometricsHelper.java (3)
50-54: Simplified fingerprint authentication methodThe
authenticateFingerprintmethod has been streamlined to delegate to the new centralizedauthenticatePinOrBiometricmethod, removing the need for theallowExtraOptionsparameter. This promotes code reuse and maintainability.
78-84: Consolidated PIN authentication implementationThe
authenticatePinmethod has been similarly refactored to use the new centralized authentication method, eliminating duplicate code and creating a more consistent authentication experience.
86-99: New unified authentication methodThe addition of
authenticatePinOrBiometriccentralizes the authentication logic for both PIN and fingerprint methods. This unified approach:
- Reduces code duplication
- Provides consistent behavior across authentication methods
- Simplifies the API by removing the need for conditional checks
- Uses a single BiometricPrompt configuration that works for both auth types
This change improves maintainability and should provide a more consistent user experience on all screen sizes.
app/res/layout/fragment_signup.xml (9)
3-3: Root Layout Migration Verified:
Changing the root element from a<FrameLayout>(implied from previous versions) to a<androidx.constraintlayout.widget.ConstraintLayout>is an excellent move to provide more flexible positioning, especially for smaller screens. Please double-check that all child views have proper constraints defined to avoid unexpected layout issues.
12-24: Top Bar Container Addition:
The new<LinearLayout>with the IDllTopBarencapsulates the included app bar (@layout/connectid_common_title_bar), which should promote consistency across screens. Verify that its constraints—as defined (start of parent and top of parent)—align with the overall design intent on various screen sizes.
41-48: Company Logo ImageView Addition:
The newly addedImageViewfor the company logo is defined with clear dimensions (a height of40dp) and a top margin (60dp), which should work well. Just ensure that these values look balanced on the smallest supported screens.
57-67: Primary Title TextView:
The customConnectBoldTextViewfor the phone title is now neatly configured with consistent margins, font family, and text color. This change enhances visual clarity.
69-79: Subtext Component Addition:
TheConnectMediumTextViewfor the phone subtext is properly set up with appropriate margins and font attributes. Ensure that the text (“ConnectId Signup”) matches the intended design language across the app.
199-227: Consent Section Layout:
The consent area that pairs aCheckBoxwith aConnectMediumTextViewshows clear separation and should enhance readability. The layout margins and text sizes are appropriate to maintain a clean interface.
229-240: Continue Button Update:
The newly added MaterialButton for the continue action is well-styled with a custom style and includes a drawable for an arrow. Verify that its touch target meets accessibility guidelines and that its placement is optimal on small screens.
247-252: Bottom Recovery Section Addition:
The introduction of the<LinearLayout>with IDll_recoverprovides a dedicated area at the bottom of the screen, constrained appropriately withapp:layout_constraintBottom_toBottomOf="parent". This should help maintain a consistent layout even on smaller devices.
290-290: ConstraintLayout Closure Confirmation:
The closing tag for theConstraintLayoutappears correctly placed to encapsulate all the content and apply the overall constraints as intended.
Product Description
Solve small screen UI issues in
Login
Delivery app download screen
Biometric and PIN/Pattern unlock screen
Technical Summary
https://dimagi.atlassian.net/browse/CCCT-829
Feature Flag
It is bug
Safety Assurance
Safety story
Mentioned changes are verified on small and large screen
Automated test coverage
QA Plan
Mentioned screen UI to be verified