-
Notifications
You must be signed in to change notification settings - Fork 358
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
refactor: [M3-8186] - Clean up DebouncedSearchTextField and fix instances where debouncing is not happening #10813
Conversation
This reverts commit b274baf.
Coverage Report: ✅ |
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.
Thanks for making this fix. I originally created the customValue
prop but it may not have been necessary.
Left some suggestions.
Failing unit tests can likely be resolved using await waitFor(() => { ... });
packages/manager/src/components/DebouncedSearchTextField/DebouncedSearchTextField.tsx
Show resolved
Hide resolved
} | ||
setQuery(value ?? ''); | ||
}, | ||
value: preselectedLinodeId ? field.value?.label ?? '' : query, |
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.
Noticing a regression here:
Steps to reproduce:
- Navigate to Linodes landing page
- Open the action menu for a linode and click "Clone Linode"
- The Linodes list in the create flow will be filtered using the source Linode's label but the search text field will display empty/placeholder.
Expected behavior:
The search text field should be pre-filled with the label of the source Linode (see prod).
@@ -152,7 +150,9 @@ export const SelectLinodePanel = (props: Props) => { | |||
expand={true} | |||
hideLabel | |||
label="" | |||
onSearch={(value) => setUserSearchText(value)} |
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.
onSearch={(value) => setUserSearchText(value)} | |
onSearch={setUserSearchText} |
onSearch={(value) => { | ||
setSearchText(value); | ||
if (searchText !== value) { | ||
setSearchText(value); | ||
} | ||
}} |
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.
This optimization is not necessary as React will perform it for us.
onSearch={(value) => { | |
setSearchText(value); | |
if (searchText !== value) { | |
setSearchText(value); | |
} | |
}} | |
onSearch={setSearchText} |
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.
Thanks for taking this one on, Chandra. Besides the thoughts Hussain raised, I'm wondering how we feel about continuing to use onSearch
as the event handler prop here in general. onChange
seems more standard to me, and I'm concerned that if we proceed with onSearch
, we'll end up with another mistake like we saw in PlacementGroupsLanding.tsx somewhere down the line.
Besides needing to update a few more files, is there any other impact that replacing theonSearch
prop with onChange
would have?
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.
Thanks for addressing feedback! This looks good, approved pending a decision regarding @mjac0bs' comment about onSearch
vs onChange
.
@hkhalil-akamai @mjac0bs Great feedback. |
|
||
import { IconButton } from '../IconButton'; | ||
|
||
import type { TextFieldProps } from 'src/components/TextField'; | ||
|
||
export interface DebouncedSearchProps extends TextFieldProps { |
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.
Okay, I see, onSearch
became an optional prop when customValue
was added, and we don't want that - onSearch
should be required. This will make it clear what the correct handler to use is. Can we make that change in this interface?
@@ -75,7 +65,7 @@ export const DebouncedSearchTextField = React.memo( | |||
return () => clearTimeout(timeout); | |||
} | |||
return undefined; | |||
}, [debounceTime, onSearch, textFieldValue]); | |||
}, [debounceTime, textFieldValue]); |
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.
This is old code, but I don't understand why we went with a useEffect and setTimeout when we generally opt to use the debounce
util throughout the codebase. And with onSearch
required, we won't need to check for it. Could we clean this up a little and achieve the same by doing?
<Textfield
[...other props]
onChange={debounce(debounceTime ?? 400, (e) => {
onSearch(e.target.value);
setTextFieldValue(e.target.value);
})}
/>
@mjac0bs Addressed the above feedback in the commit 9bb33ae
The |
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.
The only thing I found on this last pass when checking through the places that the component is used throughout CM was a console error in Longview, which seems like it's happening because inputText
can be undefined here.
I just restarted the e2e tests because they didn't kick off correctly - but approving pending the above is addressed and those look good.
Description 📝
This PR fixes the issue of debouncing is not happening for the instances of DebouncedSearchTextField component.
Changes 🔄
List any change relevant to the reviewer.
useEffect
hook to handle empty values and reduce unnecessary state updates.Target release date 🗓️
09/02
How to test 🧪
Verification steps
(How to verify changes)
PlacementGroupsLanding
,SelectLinodePanel
andLinodeSelectTable
PlacementGroupsLanding
andLinodeSelectTable
As an Author I have considered 🤔
Check all that apply