TICKET-614: Add case sensitivy to group search#7938
Conversation
Coverage Report for CI Build 25768252823Coverage increased (+0.03%) to 91.754%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats💛 - Coveralls |
donny-wong
left a comment
There was a problem hiding this comment.
Thank you @Naragod , looks good. Please update branch and I will approve it at that time. Thank you.
00a015a to
9765181
Compare
david-yz-liu
left a comment
There was a problem hiding this comment.
@Naragod Nice work! Sorry I took a while to review this. The UI is very nice and I appreciate the attention to consistency across components.
I left two comments, one of which is describes a change to the approach that causes a bit of work but I think will result in a nice simplification of these changes. Please feel free to message on Slack if you'd like to chat further about this.
|
|
||
| /** | ||
| * Locale-aware substring match with optional case sensitivity. Both arguments | ||
| * are coerced to strings, so callers do not have to guard against null/undefined. |
There was a problem hiding this comment.
I appreciate the coercion, but for null and undefined: String(null) === "null" and String(undefined) === "undefined", which I do not think are appropriate values for this function. I'd be happy to have them "coerced" into the empty string "".
| * render so the checkbox reflects the latest value without rebuilding the | ||
| * Filter — this keeps the rendered <input> elements stable across re-renders. | ||
| */ | ||
| export function caseSensitiveTextFilter({getCaseSensitive, onToggle}) { |
There was a problem hiding this comment.
Okay so looking at the overall code changes, I think we can simplify greatly by keeping the toggled "caseSensitive" state in this component rather than lift it up to the parent component. The key idea here is that we can use the onChange callback to pass arbitrary data directly between this filter component and the table.
So you should be able to do something like
export function caseSensitiveTextFilter({onChange, column}) => {
const { filterValue, setFilterValue } = React.useState("");
const { caseSensitive, setCaseSensitive } = React.useState(false);
React.useEffect(() => {
// Key line
onChange({filterValue: filterValue, caseSensitive: caseSensitive});
}, [filterValue, caseSensitive]);
return (
// The same, except the input element onChange callbacks call setFilterValue/setCaseSensitive
...
);
});Then you can modify the filterMethod functions used in the components to access filter.value.filterValue and filter.value.caseSensitive directly, rather than requiring the latter to be passed in by the parent component.
Proposed Changes
Add case sensitivity to group search.
Screenshots of your changes (if applicable)
Screen.Recording.2026-05-08.at.5.04.27.PM.mov
Associated documentation repository pull request (if applicable)
Type of Change
(Write an
Xor a brief description next to the type or types that best describe your changes.)Checklist
(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the
[ ]into a[x]in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)Before opening your pull request:
After opening your pull request:
Questions and Comments
(Include any questions or comments you have regarding your changes.)