feat: visual hierarchy for categories in the transaction filter sidebar#865
feat: visual hierarchy for categories in the transaction filter sidebar#865dgilperez wants to merge 1 commit intowe-promise:mainfrom
Conversation
|
Related Documentation No published documentation to review for changes on this repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughIntroduces a new helper method Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
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 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4efb10fe4a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| connect() { | ||
| // Build parent-child relationships map | ||
| this.childrenMap = new Map(); // parentId -> [childCheckboxes] | ||
| this.parentMap = new Map(); // childId -> parentCheckbox | ||
|
|
There was a problem hiding this comment.
Cascade initial checked parents on load
The controller only cascades when a checkbox changes, so if the page loads with a parent category already checked (e.g., from a saved search or URL params), its subcategories remain unchecked and won’t be submitted. Because the backend change now filters only explicitly selected categories, those preselected parent-only filters will silently stop including subcategory transactions. Consider applying the cascade once on connect for any already-checked parents so initial state matches the intended “parent implies children” behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch! Fixed in 8675ad5 — added applyInitialCascade() that runs on connect to check children of any pre-selected parents. 👍
8675ad5 to
a2f5947
Compare
Adds a `family_categories_with_hierarchy` view helper (parents first, each followed by its subcategories) and renders the filter checkbox list with a hanging indent plus a `corner-down-right` glyph on children, so the parent→subcategory relationship is visible without having to trace the category badges. No changes to filter semantics: the server still auto-includes subcategories when a parent is selected (current main behavior). This PR addresses only the transparency gap — users who don't know how filtering works can now see the hierarchy in the UI itself. Closes the scope that was originally bundled with the cascading-checkbox refactor and backend-change in the earlier we-promise#865 attempt. Reshaped after review feedback: the cascading UX is tied to the (stalled) we-promise#829 backend change, which decouples parent→children filter semantics — without that merged, a cascading checkbox UI is redundant with main's auto-inclusion behavior. Keeping just the hierarchy-visibility piece, which is useful and non-controversial regardless of the semantic question we-promise#829 was raising.
a2f5947 to
4222e78
Compare
|
Reshaped after a long pause, sorry for the silence 🙏
what do you think @jjmata ? still relevant? |
|
I like the hierarchical visual of the filter, definitely easier to understand and to find the right category. However, I agree that the behavior might be misleading without the back-end change on how the filters are applied. Let's wait for @jjmata’s feedback on this 😉 |
|
Happy to move forward with both changes, but it all depends on your ability to finish the other draft PR @alessiocappa! |
I believe this was marked as draft while waiting for a final decision on the implementation, and to have it ready in advance. It should already be good to merge, but please let me know if anything else is needed. |
Summary
Adds a
family_categories_with_hierarchyview helper (parents first, each followed by its subcategories) and renders the transaction filter checkbox list with a hanging indent plus acorner-down-rightglyph on children. Users can now see the parent→subcategory relationship in the filter UI itself without having to trace the category badges.No filter semantics change. The server still auto-includes subcategories when a parent is selected (current main behavior). This PR closes only the transparency gap.
Why this scope
This PR originally bundled three concerns:
apply_category_filterthat stops auto-including subcategories when a parent is checked ❌ droppedConcerns 2 and 3 were coupled to @alessiocappa's approved-but-stalled #829 ("Exclude subcategories from transactions filter"). Without #829 merged, the cascading UI is redundant with main's current auto-inclusion behavior — checking "Food" already shows Food + Restaurants transactions server-side; auto-checking the child boxes in JS would be decorative, not functional.
Keeping just the hierarchy-visibility piece, which:
Changes
app/helpers/categories_helper.rb— newfamily_categories_with_hierarchyhelper.app/views/transactions/searches/filters/_category_filter.html.erb— use the new helper, addml-4and acorner-down-righticon for subcategories.test/helpers/categories_helper_test.rb— cover both helpers (alphabetical ordering + parent-before-child invariant).Tests
bin/rubocop+bin/brakemancleanSummary by CodeRabbit