JNG-6075 improve card filters #502
Conversation
noherczeg
commented
Jan 2, 2025
•
noherczeg
commented
 JNG-6075 Allow Card Filters to support text filters not just options
|
noherczeg
requested review from
robertcsakany,
bencegelei,
gaborflorian and
Madmat8
WalkthroughThe changes to the Changes
Sequence DiagramsequenceDiagram
participant User
participant CardsFilter
participant SearchInput
participant FilterOptions
User->>SearchInput: Enter search term
SearchInput->>CardsFilter: Trigger debounced update
CardsFilter->>FilterOptions: Filter visible options
FilterOptions-->>User: Display filtered results
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
judo-ui-react/src/main/resources/actor/src/components/widgets/CardsFilter.tsx.hbs (2)
74-76: Ensure consistent handling ofallowSearch.
WhenallowSearchis disabled, users will only see the selectable values. This design is logical. Just remember to occasionally confirm that the user experience remains consistent when togglingallowSearchfrom true to false, especially in edge cases if there's partial input present insearchValues.
77-93: Validate potential performance costs of defaultValue usage.
By passingdefaultValue={searchValues[d.field as string]}, theTextFieldcomponent doesn’t fully control its state internally, which can lead to unexpected re-render behavior if end users switch filters rapidly. Switching to a controlled component by usingvalueinstead ofdefaultValueis typically recommended unless there's a specific reason.- defaultValue={searchValues[d.field as string]} + value={searchValues[d.field as string] ?? ''}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
judo-ui-react/src/main/resources/actor/src/components/widgets/CardsFilter.tsx.hbs(3 hunks)
🔇 Additional comments (5)
judo-ui-react/src/main/resources/actor/src/components/widgets/CardsFilter.tsx.hbs (5)
23-26: Great expansion of theCardsFilterDefinitioninterface.
The new properties (inputType,searchLabel,allowSearch,values) are well-defined and optional. This improved flexibility is useful for handling both text-based and selectable filter types.
34-35: Clarify the role ofvisibleValuesstate.
WhilesearchValuesis clearly used to store the user input for searching,visibleValuesis set up but never updated in this code snippet. Ensure you plan to either maintain it for future enhancements or remove it to avoid confusion and potential memory overhead.
46-49: Well-structured debounced update for performance.
This approach conveniently prevents spamming the state updates with every keystroke.
94-116: Appropriate integration of text-based filtering with checkboxes.
By filtering the list in.filter(...)using thesearchValues, you let users narrow the displayed options quickly. This is a neat hybrid approach to searching arrangement-based filters. No issues are apparent, though be mindful of extremely largevaluesarrays, as continuous filtering can get heavy.
11-12: Consider verifying the size of the@mui/material/utilspackage.
Importingdebouncefrom@mui/material/utilsis permissible, but for performance-critical paths, you could consider an alternative, lightweight utility, if there's a need to reduce bundle size. Otherwise, this is fine.Run the following script to check if
debounceis imported elsewhere and to estimate its overall usage:✅ Verification successful
debounceusage is consistent with project patternsThe codebase shows a consistent pattern where
debouncefrom@mui/material/utilsis used in conjunction with a localdebounceInputsconfiguration in multiple UI components that handle user input:
- CardsFilter.tsx.hbs
- Tags.tsx.hbs
- TextWithTypeAhead.tsx.hbs
- SingleRelationInput.tsx.hbs
This appears to be an intentional architectural decision to use Material-UI's debounce implementation across input components, making it an integral part of the project's UI framework rather than a one-off usage. Given this consistent pattern and integration with the project's configuration (
debounceInputs), switching to an alternative implementation would require broader architectural changes without clear benefits.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # This script checks for other references to `@mui/material/utils` and displays context lines. rg -A 5 "@mui/material/utils"Length of output: 3134
