SAAS-18493 Tighten DateTimeWidget to a compact calendar-only layout#3728
Draft
avazirna wants to merge 3 commits into
Draft
SAAS-18493 Tighten DateTimeWidget to a compact calendar-only layout#3728avazirna wants to merge 3 commits into
avazirna wants to merge 3 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR makes three targeted updates to Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/org/commcare/views/widgets/DateTimeWidget.java (1)
96-102: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRename method to reflect that it now sets both dimensions.
The method
updateCalendarViewHeight()now sets bothwidthandheighttoWRAP_CONTENT, but the name only mentions height. This is misleading for future maintainers.♻️ Suggested refactor to rename the method
- private void updateCalendarViewHeight() { + private void updateCalendarViewDimensions() { CalendarView calendarView = mDatePicker.getCalendarView(); LayoutParams params = (LayoutParams) calendarView.getLayoutParams(); params.height = LayoutParams.WRAP_CONTENT; params.width = LayoutParams.WRAP_CONTENT; calendarView.setLayoutParams(params); }Also update the call site on line 42:
mDatePicker.setSpinnersShown(false); - updateCalendarViewHeight(); + updateCalendarViewDimensions();🤖 Prompt for 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. In `@app/src/org/commcare/views/widgets/DateTimeWidget.java` around lines 96 - 102, Rename the misleading method updateCalendarViewHeight() to a name reflecting both dimensions (e.g., updateCalendarViewSize or setCalendarViewWrapContent) and update all call sites to the new method name; inside the renamed method keep the existing body that gets the CalendarView via mDatePicker.getCalendarView(), casts LayoutParams, sets params.height and params.width to LayoutParams.WRAP_CONTENT, and reapplies them with calendarView.setLayoutParams(params).
🧹 Nitpick comments (1)
app/src/org/commcare/views/widgets/DateTimeWidget.java (1)
92-94: ⚡ Quick winUpdate comment to reflect that width is also being set.
The comment describes the workaround as updating "the calendarview height to wrap content", but now the method also sets the width. The comment should be updated for accuracy.
📝 Suggested comment update
/** - * CalendarView bottom line gets cut off in appcompat theme because it's height is fixed here: + * CalendarView bottom line gets cut off and stretches to match parent in appcompat theme + * because its dimensions are fixed here: * https://github.com/aosp-mirror/platform_frameworks_base/blob/master/core/res/res/layout/date_picker_legacy.xml#L78 - * This workaround updates the calendarview height to wrap content. + * This workaround updates the calendarview dimensions to wrap content. */🤖 Prompt for 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. In `@app/src/org/commcare/views/widgets/DateTimeWidget.java` around lines 92 - 94, The existing comment above the workaround in DateTimeWidget.java currently states it "updates the calendarview height to wrap content" but the code also adjusts the width; update that comment to accurately describe both dimensions being changed (height and width) and mention that the workaround sets calendarView layout params to wrap_content for height and width so readers of the DateTimeWidget class understand the full effect.
🤖 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.
Outside diff comments:
In `@app/src/org/commcare/views/widgets/DateTimeWidget.java`:
- Around line 96-102: Rename the misleading method updateCalendarViewHeight() to
a name reflecting both dimensions (e.g., updateCalendarViewSize or
setCalendarViewWrapContent) and update all call sites to the new method name;
inside the renamed method keep the existing body that gets the CalendarView via
mDatePicker.getCalendarView(), casts LayoutParams, sets params.height and
params.width to LayoutParams.WRAP_CONTENT, and reapplies them with
calendarView.setLayoutParams(params).
---
Nitpick comments:
In `@app/src/org/commcare/views/widgets/DateTimeWidget.java`:
- Around line 92-94: The existing comment above the workaround in
DateTimeWidget.java currently states it "updates the calendarview height to wrap
content" but the code also adjusts the width; update that comment to accurately
describe both dimensions being changed (height and width) and mention that the
workaround sets calendarView layout params to wrap_content for height and width
so readers of the DateTimeWidget class understand the full effect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0290041f-b1f3-4de9-bdb5-0b4fdd15cc9a
📒 Files selected for processing (1)
app/src/org/commcare/views/widgets/DateTimeWidget.java
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3728 +/- ##
============================================
- Coverage 23.36% 23.33% -0.03%
Complexity 3927 3927
============================================
Files 923 923
Lines 56058 56094 +36
Branches 6641 6643 +2
============================================
- Hits 13096 13092 -4
- Misses 41268 41307 +39
- Partials 1694 1695 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Product Description
Cleans up the layout of the date+time form widget. Previously the
DatePickerrendered both its month/day/year spinner header and the full CalendarView, and the calendar stretched to fill the form's width, producing a tall picker with cramped day cells. After this PR:The TimePicker portion of the widget is unchanged.
Technical Summary
Ticket: https://dimagi.atlassian.net/browse/SAAS-18493 — "Request to update date & time widget formatting" (Bug).
The change is purely visual; no answer/serialization behavior is touched. Both
setSpinnersShownandsetCalendarViewShownhave been stable AndroidDatePickerAPIs since API 11.Safety Assurance
Safety story
DateTimeDataanswer with the selected date and time, and that read-only forms still disable the pickers (setFocusable/setEnabledpaths unchanged).Automated test coverage
No new automated coverage — the change is a layout/visual tweak that existing unit tests don't cover and that's difficult to assert on without a screenshot test harness. Existing form-rendering smoke tests will catch any inflation/crash regressions.
Labels and Review