Skip to content

Conversation

@Jignesh-dimagi
Copy link
Contributor

Technical Summary

https://dimagi.atlassian.net/browse/CI-391

As per the ticket, user is having small screen phone so not able to view the SAVE PHOTO button after taking the photo. In this current changes, I have made both button on same line equi distance from center as showing in below screen shot

Screenshot_20251118_163551

Feature Flag

Safety Assurance

QA Plan

Test upload photo

Labels and Review

  • Do we need to enhance the manual QA test coverage ? If yes, the "QA Note" label is set correctly
  • Does the PR introduce any major changes worth communicating ? If yes, the "Release Note" label is set and a "Release Note" is specified in PR description.
  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@coderabbitai
Copy link

coderabbitai bot commented Nov 18, 2025

📝 Walkthrough

Walkthrough

This change restructures the button layout in the personal ID photo capture screen by introducing a horizontal LinearLayout container that wraps two MaterialButton controls. Both buttons are configured with weight-based widths (0dp with weight=1) to distribute horizontal space equally, with start and end margins added for spacing between them. The second button remains disabled by default.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Single file modification with straightforward layout restructuring
  • Weight-based width changes to buttons for equal space distribution
  • Margin adjustments for button spacing

Suggested labels

Release Note

Suggested reviewers

  • OrangeAndGreen
  • jaypanchal-13

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: solving a small screen size issue where the SAVE PHOTO button was hidden during personal ID photo capture, matching the changeset which restructures button layout.
Description check ✅ Passed The description includes technical summary with ticket link, problem description, and visual evidence, but lacks complete Safety Assurance section (missing safety story and automated test coverage details).
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch jignesh/fix/ci-391

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
app/res/layout/screen_personalid_photo_capture.xml (1)

52-59: Optional: Remove redundant gravity attribute.

The android:gravity="center_horizontal" on the horizontal LinearLayout is redundant since both child buttons use layout_weight="1", occupying the full available width. This attribute has no functional effect but can be removed for clarity.

         <LinearLayout
             android:layout_width="match_parent"
             android:layout_height="wrap_content"
             android:orientation="horizontal"
-            android:gravity="center_horizontal"
             android:layout_marginEnd="20dp"
             android:layout_marginStart="20dp"
             >
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2c3dc74 and 0ad61ac.

📒 Files selected for processing (1)
  • app/res/layout/screen_personalid_photo_capture.xml (1 hunks)
🔇 Additional comments (1)
app/res/layout/screen_personalid_photo_capture.xml (1)

52-76: Layout restructuring correctly resolves small-screen visibility issue.

The weight-based layout approach is the correct standard pattern for distributing equal horizontal space to both buttons. The new horizontal LinearLayout wrapper (lines 52-76) with layout_weight="1" on each button ensures both are visible and properly sized across device widths, directly addressing the reported issue. Spacing is appropriate: 20dp container margins and 5dp inter-button gap maintain visual balance.

Copy link
Contributor

@conroy-ricketts conroy-ricketts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that Coderabbit's nitpick comment is worth looking at, but these changes look good to me.

This ticket is very interesting to me though... I wonder if that user is not able to do a lot of different things in the CommCare app because of the abnormal screen size/ratio? Surely this wouldn't be his only issue?

@Jignesh-dimagi
Copy link
Contributor Author

I think that Coderabbit's nitpick comment is worth looking at, but these changes look good to me.

This ticket is very interesting to me though... I wonder if that user is not able to do a lot of different things in the CommCare app because of the abnormal screen size/ratio? Surely this wouldn't be his only issue?

@conroy-ricketts I was also having same thought that it was something else but after debugging found out that the screen size is the issue.

Initial pages on PersonalId registration has screen component with less gap between them. I tried to replicate the issue with small screen emulator, it shows clearly that screen has no space to show the Save Photo button.

Screenshot_20251119_114252

@Jignesh-dimagi Jignesh-dimagi merged commit 0f38100 into commcare_2.61 Nov 19, 2025
2 checks passed
@Jignesh-dimagi Jignesh-dimagi deleted the jignesh/fix/ci-391 branch November 19, 2025 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-integration-tests Skip android tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants