-
Notifications
You must be signed in to change notification settings - Fork 4
fix: add debounced prop and ux to mui search input component #209
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
10075aa
baef896
39bac29
0dd6a00
9f8f4b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,53 +11,67 @@ | |
| * limitations under the License. | ||
| * */ | ||
|
|
||
| import React, { useEffect, useState } from "react"; | ||
| import { TextField, IconButton } from "@mui/material"; | ||
| import React, { useEffect, useState, useMemo } from "react"; | ||
| import { TextField, IconButton, InputAdornment } from "@mui/material"; | ||
| import SearchIcon from "@mui/icons-material/Search"; | ||
| import ClearIcon from "@mui/icons-material/Clear"; | ||
| import { debounce } from "lodash"; | ||
| import { DEBOUNCE_WAIT } from "../../utils/constants"; | ||
|
|
||
| const SearchInput = ({ term, onSearch, placeholder = "Search..." }) => { | ||
| const SearchInput = ({ term, onSearch, placeholder = "Search...", debounced }) => { | ||
| const [searchTerm, setSearchTerm] = useState(term); | ||
|
|
||
| useEffect(() => { | ||
| setSearchTerm(term || ""); | ||
| }, [term]); | ||
|
|
||
| const handleSearch = (ev) => { | ||
| if (ev.key === "Enter") { | ||
| onSearch(searchTerm); | ||
| } | ||
| }; | ||
|
|
||
| const handleClear = () => { | ||
| onSearchDebounced?.cancel(); | ||
| setSearchTerm(""); | ||
| onSearch(""); | ||
| }; | ||
|
|
||
| const onSearchDebounced = useMemo( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tomrndom Why the existing tests miss it render(<SearchInput term="" onSearch={onSearch} debounced />);In a test, onSearch is a stable jest.fn() reference and there are no parent re-renders. So useMemo never recomputes, the debounced function is never cancelled, and the timer fires normally. The test passes. Production breaks. Proposed fix Replace the current memoization with a ref-stable wrapper so the debounced function is created once per debounced value and is not destroyed by parent re-renders, while still calling the latest |
||
| () => debounced ? debounce((value) => onSearch(value), DEBOUNCE_WAIT) : null, | ||
| [onSearch, debounced] | ||
| ); | ||
|
|
||
| useEffect(() => () => onSearchDebounced?.cancel(), [onSearchDebounced]); | ||
|
|
||
| const handleChange = (value) => { | ||
| setSearchTerm(value); | ||
| if (debounced) onSearchDebounced(value); | ||
| }; | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| const handleKeyDown = (ev) => { | ||
| if (!debounced && ev.key === "Enter") { | ||
| onSearch(searchTerm); | ||
| } | ||
| }; | ||
|
|
||
| return ( | ||
| <TextField | ||
| variant="outlined" | ||
| value={searchTerm} | ||
| placeholder={placeholder} | ||
| slotProps={{ | ||
| input: { | ||
| endAdornment: term ? ( | ||
| <IconButton | ||
| size="small" | ||
| onClick={handleClear} | ||
| sx={{ position: "absolute", right: 0 }} | ||
| > | ||
| <ClearIcon sx={{ color: "#0000008F" }} /> | ||
| </IconButton> | ||
| ) : ( | ||
| <SearchIcon | ||
| sx={{ mr: 1, color: "#0000008F", position: "absolute", right: 0 }} | ||
| /> | ||
| startAdornment: ( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tomrndom |
||
| <InputAdornment position="start"> | ||
| <SearchIcon sx={{ color: "#0000008F" }} /> | ||
| </InputAdornment> | ||
| ), | ||
| endAdornment: term && ( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tomrndom |
||
| <InputAdornment position="end"> | ||
| <IconButton size="small" onClick={handleClear}> | ||
| <ClearIcon fontSize="small" sx={{ color: "#0000008F" }} /> | ||
| </IconButton> | ||
| </InputAdornment> | ||
| ) | ||
| } | ||
| }} | ||
| onChange={(event) => setSearchTerm(event.target.value)} | ||
| onKeyDown={handleSearch} | ||
| onChange={(ev) => handleChange(ev.target.value)} | ||
| onKeyDown={handleKeyDown} | ||
| fullWidth | ||
| sx={{ | ||
| "& .MuiOutlinedInput-root": { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomrndom handleClear references onSearchDebounced before it is declared