CCCT-2515 Add Reusable Semi-Circle Progress Bar#3779
Conversation
Suggested Review Order
|
📝 WalkthroughWalkthroughA new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/org/commcare/views/connect/SemiCircleArcView.kt`:
- Around line 78-82: The width measurement in SemiCircleArcView’s sizing logic
treats MeasureSpec.AT_MOST the same as EXACTLY, which makes wrap_content use the
parent’s full available width instead of the view’s intrinsic size. Update the
measurement branch in the width/height calculation to treat AT_MOST as a cap
over the default dimension rather than directly returning MeasureSpec.getSize,
and keep the intrinsic default from resources as the preferred size in
SemiCircleArcView.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 06dceec0-5aa5-46da-9b54-677f045e5091
📒 Files selected for processing (16)
app/res/layout/view_semi_circle_progress_bar.xmlapp/res/values-es/strings.xmlapp/res/values-fr/strings.xmlapp/res/values-ha/strings.xmlapp/res/values-hi/strings.xmlapp/res/values-lt/strings.xmlapp/res/values-no/strings.xmlapp/res/values-pt/strings.xmlapp/res/values-sw/strings.xmlapp/res/values-ti/strings.xmlapp/res/values/attrs.xmlapp/res/values/dimens.xmlapp/res/values/strings.xmlapp/res/values/styles.xmlapp/src/org/commcare/views/connect/SemiCircleArcView.ktapp/src/org/commcare/views/connect/SemiCircleProgressBar.kt
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3779 +/- ##
============================================
- Coverage 25.84% 25.82% -0.02%
- Complexity 4387 4427 +40
============================================
Files 951 966 +15
Lines 57199 57851 +652
Branches 6812 6884 +72
============================================
+ Hits 14782 14939 +157
- Misses 40590 41075 +485
- Partials 1827 1837 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Treat AT_MOST as a cap over the 240dp default instead of filling the available width, so wrap_content no longer behaves like match_parent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
OrangeAndGreen
requested review from
Jignesh-dimagi,
conroy-ricketts and
shubham1g5
|
Out of curiosity, was the design change intentional (i.e. using X of Y rather than X/Y)? |
| <string name="connect_progress_delivery">Delivery</string> | ||
| <string name="connect_progress_title">Delivery Progress</string> | ||
| <string name="connect_progress_status">You have completed %d of %d visits.</string> | ||
| <string name="semi_circle_progress_value_format" cc:translatable="true">%1$d of %2$d</string> |
There was a problem hiding this comment.
Can we name this connect_progress_text instead
And confirming that this needs to be cc:translatable?
| android:layout_height="wrap_content" | ||
| android:textSize="@dimen/semi_circle_progress_value_text_size" | ||
| android:textStyle="bold" | ||
| tools:text="25 of 40" /> |
There was a problem hiding this comment.
Was this tested in the Android Studio preview window? Trying to gauge how valuable tools:text is in this layout. We could maybe go without it
| <attr name="strokeWidth" format="dimension" /> | ||
| </declare-styleable> | ||
|
|
||
| <declare-styleable name="SemiCircleProgressBar"> |
There was a problem hiding this comment.
Curious how the team feels about prefixing these with Connect
| <dimen name="semi_circle_progress_default_width">240dp</dimen> | ||
| <dimen name="semi_circle_progress_stroke_width">14dp</dimen> | ||
| <dimen name="semi_circle_progress_value_text_size">28sp</dimen> | ||
| <dimen name="semi_circle_progress_description_text_size">16sp</dimen> |
There was a problem hiding this comment.
Similar comment as above, are these worth prefixing with connect like we do strings currently?
There was a problem hiding this comment.
Noting that there is a contrast with how it was defined for the Connect info card
@conroy-ricketts Confirmed, the design changed slightly in the final Figma so I went with the latest from there |
2 participants
CCCT-2515
Technical Summary
Adds a reusable 180° semi-circle progress indicator. It is split into two pieces:
SemiCircleArcView— a focused customViewthat draws only the track + progress arcs (exposes aprogressfraction, arc colors, and stroke width).SemiCircleProgressBar— aConstraintLayoutcomposite that overlays two centeredTextViews (a "X of Y" value line and a description line) on the arc, so text centering/rendering is handled by the layout rather than canvas math.The four colors (
progressColor,trackColor,valueTextColor,descriptionTextColor) are mapped in one place via theWidget.CommCare.SemiCircleProgressBarstyle;current,max, anddescriptionTextare the per-instance content. The value line uses a localized format string (semi_circle_progress_value_format) translated across all supported languages.Note that the active value is clipped between 0 and the max, i.e.:
Also, the max value is floored at 0 (to handle negative inputs) and will always show an empty progress bar when drawing with max=0. Since the current value is clipped between (0, max), the text will always read "0 of 0" when max=0.
This PR adds the component only — it is not yet wired into any production screen.
Demo of the new component (temporarily added to Learning Progress and hard-coded with some sample data):
Safety Assurance
Safety story
What gives me confidence: