Staggered Triple Stack Visualization#212
Open
amorris13 wants to merge 3 commits into
Open
Conversation
…d overlap Modified `TripleStackView` and `FoundTriplesView` to implement a 10% horizontal offset between cards in a stack and increased the vertical overlap to 45% (a 10% increase). This change makes the triple stack visuals more closely resemble the app's logo and look more like a physical stack of cards. Updated dimension calculations, drawing logic, and touch bounds to ensure consistent layout and animations across the app. Verified the changes via Roborazzi screenshots and existing unit tests. Co-authored-by: amorris13 <4523811+amorris13@users.noreply.github.com>
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds horizontal stack displacement to triple card layouts: card width, stack horizontal displacement, measurement, rendering transforms, animation target bounds, and outline/clipping are updated across three view classes to position each card with an i * horizontal offset and adjusted vertical overlap. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/main/java/com/antsapps/triples/views/TripleStackView.java (1)
90-92: A small stack-bounds helper would trim repeated geometry math.
mCardWidth + 2 * mStackHorizDisplacementand the sameRectFedges are rebuilt in the outline, highlight, and placeholder paths. Pulling that into one helper would make the next stack-shape tweak a little safer.Also applies to: 207-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/antsapps/triples/views/TripleStackView.java` around lines 90 - 92, Extract the repeated geometry math into a small helper on TripleStackView (e.g., getStackBounds() or computeStackRect()) that uses mPadding, mCardWidth, mStackHorizDisplacement and totalHeight to return a RectF (or the left/top/right/bottom values) representing the stack rectangle; then replace the duplicated expressions (mCardWidth + 2 * mStackHorizDisplacement and the repeated RectF edges used in outline.setRoundRect, the highlight path, and the placeholder path) with calls to that helper so all three places reuse the same computed bounds.app/src/main/java/com/antsapps/triples/views/FoundTriplesView.java (1)
202-208: Consider sharing the stack geometry calculation.
FoundTriplesViewnow copies the same card-width and displacement math thatTripleStackView.updateDimensions()already owns. Centralizing that logic would reduce the chance of parent-computed animation targets drifting from the child’s actual render geometry on the next visual tweak.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/antsapps/triples/views/FoundTriplesView.java` around lines 202 - 208, FoundTriplesView duplicates the card/displacement math from TripleStackView.updateDimensions(), causing drift risk; replace the inline calculations for mCardWidth, mCardHeight, mStackDisplacement and mStackHorizDisplacement with a call to a shared helper on TripleStackView (e.g., add a public static method like TripleStackView.calculateStackGeometry(int slotWidth, int padding) or expose an accessor that returns computed width/height/displacements) and use its returned values (based on CARD ratios, TripleStackView.STACK_DISPLACEMENT_PERCENT, STACK_HORIZ_DISPLACEMENT_PERCENT and CardView.HEIGHT_OVER_WIDTH) instead of recomputing from mSlotWidth and mPadding in FoundTriplesView so both parent and child use the same source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/com/antsapps/triples/views/FoundTriplesView.java`:
- Around line 202-208: FoundTriplesView duplicates the card/displacement math
from TripleStackView.updateDimensions(), causing drift risk; replace the inline
calculations for mCardWidth, mCardHeight, mStackDisplacement and
mStackHorizDisplacement with a call to a shared helper on TripleStackView (e.g.,
add a public static method like TripleStackView.calculateStackGeometry(int
slotWidth, int padding) or expose an accessor that returns computed
width/height/displacements) and use its returned values (based on CARD ratios,
TripleStackView.STACK_DISPLACEMENT_PERCENT, STACK_HORIZ_DISPLACEMENT_PERCENT and
CardView.HEIGHT_OVER_WIDTH) instead of recomputing from mSlotWidth and mPadding
in FoundTriplesView so both parent and child use the same source of truth.
In `@app/src/main/java/com/antsapps/triples/views/TripleStackView.java`:
- Around line 90-92: Extract the repeated geometry math into a small helper on
TripleStackView (e.g., getStackBounds() or computeStackRect()) that uses
mPadding, mCardWidth, mStackHorizDisplacement and totalHeight to return a RectF
(or the left/top/right/bottom values) representing the stack rectangle; then
replace the duplicated expressions (mCardWidth + 2 * mStackHorizDisplacement and
the repeated RectF edges used in outline.setRoundRect, the highlight path, and
the placeholder path) with calls to that helper so all three places reuse the
same computed bounds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 358267f5-58ae-4b13-a740-701b83d2d599
⛔ Files ignored due to path filters (35)
app/src/test/screenshots/arcade_game_dark.pngis excluded by!**/*.pngapp/src/test/screenshots/arcade_game_light.pngis excluded by!**/*.pngapp/src/test/screenshots/beginner_mode_dark.pngis excluded by!**/*.pngapp/src/test/screenshots/beginner_mode_light.pngis excluded by!**/*.pngapp/src/test/screenshots/classic_game_analysis_dark.pngis excluded by!**/*.pngapp/src/test/screenshots/classic_game_analysis_light.pngis excluded by!**/*.pngapp/src/test/screenshots/classic_game_dark.pngis excluded by!**/*.pngapp/src/test/screenshots/classic_game_light.pngis excluded by!**/*.pngapp/src/test/screenshots/daily_game_dark.pngis excluded by!**/*.pngapp/src/test/screenshots/daily_game_light.pngis excluded by!**/*.pngapp/src/test/screenshots/help_dark.pngis excluded by!**/*.pngapp/src/test/screenshots/help_light.pngis excluded by!**/*.pngapp/src/test/screenshots/incorrect_triples_snackbar.pngis excluded by!**/*.pngapp/src/test/screenshots/main_dark.pngis excluded by!**/*.pngapp/src/test/screenshots/main_light.pngis excluded by!**/*.pngapp/src/test/screenshots/settings_dark.pngis excluded by!**/*.pngapp/src/test/screenshots/settings_light.pngis excluded by!**/*.pngapp/src/test/screenshots/statistics_arcade_analysis_dark.pngis excluded by!**/*.pngapp/src/test/screenshots/statistics_arcade_analysis_light.pngis excluded by!**/*.pngapp/src/test/screenshots/statistics_arcade_dark.pngis excluded by!**/*.pngapp/src/test/screenshots/statistics_arcade_light.pngis excluded by!**/*.pngapp/src/test/screenshots/statistics_classic_analysis_dark.pngis excluded by!**/*.pngapp/src/test/screenshots/statistics_classic_analysis_light.pngis excluded by!**/*.pngapp/src/test/screenshots/statistics_classic_dark.pngis excluded by!**/*.pngapp/src/test/screenshots/statistics_classic_light.pngis excluded by!**/*.pngapp/src/test/screenshots/statistics_classic_partial_analysis_dark.pngis excluded by!**/*.pngapp/src/test/screenshots/statistics_classic_partial_analysis_light.pngis excluded by!**/*.pngapp/src/test/screenshots/view_board_dark.pngis excluded by!**/*.pngapp/src/test/screenshots/view_board_light.pngis excluded by!**/*.pngapp/src/test/screenshots/zen_game_dark.pngis excluded by!**/*.pngapp/src/test/screenshots/zen_game_hint_dark.pngis excluded by!**/*.pngapp/src/test/screenshots/zen_game_hint_light.pngis excluded by!**/*.pngapp/src/test/screenshots/zen_game_hint_selected_dark.pngis excluded by!**/*.pngapp/src/test/screenshots/zen_game_hint_selected_light.pngis excluded by!**/*.pngapp/src/test/screenshots/zen_game_light.pngis excluded by!**/*.png
📒 Files selected for processing (2)
app/src/main/java/com/antsapps/triples/views/FoundTriplesView.javaapp/src/main/java/com/antsapps/triples/views/TripleStackView.java
Implemented a 10% horizontal displacement in `TripleStackView` and `FoundTriplesView` to mimic the app logo and create a more natural stack appearance. Also added horizontal displacement to cards in the `TripleExplanationView` for visual consistency. Reverted the vertical overlap changes, maintaining the original 65% displacement (35% overlap). Updated layout logic and bounds calculations to ensure correct sizing and animations. Verified the changes with updated Roborazzi screenshot tests. Co-authored-by: amorris13 <4523811+amorris13@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/com/antsapps/triples/views/TripleExplanationView.java (1)
81-89: Fixed dp margins may not match the claimed "10% of card width" across screen sizes.The comment states this mimics a "10% of card width" shift, but the implementation uses fixed 3dp increments. In
TripleStackView, the horizontal displacement is calculated dynamically asmCardWidth * 0.10f. Depending on the actual card width in this view,3dpmay not equal 10% of the card width, potentially causing visual inconsistency between the explanation view and the stack view on different screen densities or sizes.Consider calculating the margin dynamically based on the card's measured width, similar to how
TripleStackViewcalculatesmStackHorizDisplacement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/antsapps/triples/views/TripleExplanationView.java` around lines 81 - 89, The fixed 3dp margins in TripleExplanationView don't reflect the "10% of card width" shift and can diverge across devices; change the margin calculation to compute horizontal displacement from the actual measured card width (like TripleStackView's mStackHorizDisplacement = cardWidth * 0.10f) instead of dpToPx(3). After the card views are measured (e.g., in onLayout/onMeasure or by posting a Runnable after layout), read the card width (from mCardViews[i].getMeasuredWidth() or the container width divided by cell count), compute int horiz = Math.round(cardWidth * 0.10f), and set cardParams.leftMargin/rightMargin using horiz * i and horiz * (2 - i) before calling mCardViews[i].setLayoutParams(cardParams); keep using dpToPx only for true fixed paddings, not for this proportional offset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/com/antsapps/triples/views/TripleExplanationView.java`:
- Around line 81-89: The fixed 3dp margins in TripleExplanationView don't
reflect the "10% of card width" shift and can diverge across devices; change the
margin calculation to compute horizontal displacement from the actual measured
card width (like TripleStackView's mStackHorizDisplacement = cardWidth * 0.10f)
instead of dpToPx(3). After the card views are measured (e.g., in
onLayout/onMeasure or by posting a Runnable after layout), read the card width
(from mCardViews[i].getMeasuredWidth() or the container width divided by cell
count), compute int horiz = Math.round(cardWidth * 0.10f), and set
cardParams.leftMargin/rightMargin using horiz * i and horiz * (2 - i) before
calling mCardViews[i].setLayoutParams(cardParams); keep using dpToPx only for
true fixed paddings, not for this proportional offset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 837e20d0-dd55-4764-9254-2b30d6f3c182
⛔ Files ignored due to path filters (10)
app/src/test/screenshots/beginner_mode_dark.pngis excluded by!**/*.pngapp/src/test/screenshots/beginner_mode_light.pngis excluded by!**/*.pngapp/src/test/screenshots/classic_game_analysis_dark.pngis excluded by!**/*.pngapp/src/test/screenshots/classic_game_analysis_light.pngis excluded by!**/*.pngapp/src/test/screenshots/daily_game_dark.pngis excluded by!**/*.pngapp/src/test/screenshots/daily_game_light.pngis excluded by!**/*.pngapp/src/test/screenshots/help_dark.pngis excluded by!**/*.pngapp/src/test/screenshots/help_light.pngis excluded by!**/*.pngapp/src/test/screenshots/view_board_dark.pngis excluded by!**/*.pngapp/src/test/screenshots/view_board_light.pngis excluded by!**/*.png
📒 Files selected for processing (2)
app/src/main/java/com/antsapps/triples/views/TripleExplanationView.javaapp/src/main/java/com/antsapps/triples/views/TripleStackView.java
amorris13
commented
Mar 24, 2026
| // Since the card cell takes 1/5th of the row width, a 10% shift of the card is | ||
| // effectively a shift of 0.1 * (1/5) = 2% of the total row width. | ||
| // We apply this using margins to avoid drawing outside the cell bounds. | ||
| cardParams.leftMargin = dpToPx(i * 3); |
Owner
Author
There was a problem hiding this comment.
Ideally this 3 (and the -13 for the topMargin above) would instead be calculated based on the measured width of the card, rather than just hardcoded. Is that possible?
Contributor
There was a problem hiding this comment.
I've updated TripleExplanationView.java to calculate these margins dynamically in onMeasure. The horizontal and vertical displacements now use the constants from TripleStackView and the measured width of the view, ensuring visual consistency without hardcoded values.
Updated `TripleExplanationView.java` to calculate horizontal and vertical margins dynamically in `onMeasure` using the card width and constants from `TripleStackView`. This ensures the staggered effect is consistent across different screen sizes and avoids hardcoded pixel values. Also fixed a missing import for `CardView` and verified the visuals with updated Roborazzi screenshots. Co-authored-by: amorris13 <4523811+amorris13@users.noreply.github.com>
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.
Updated the visual appearance of card stacks in the game to include a 10% horizontal offset and a 45% vertical overlap, matching the app's logo and providing a more realistic "stack" look. Adjusted
TripleStackViewandFoundTriplesViewto support these changes while maintaining proper layout and animation targets.PR created automatically by Jules for task 5173821560565866598 started by @amorris13
Summary by CodeRabbit