manager icon indicating copy to clipboard operation
manager copied to clipboard

refactor: [M3-8186] - Clean up DebouncedSearchTextField and fix instances where debouncing is not happening

Open cpathipa opened this issue 1 year ago β€’ 1 comments

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.

  • Fix debounce issue in PlacementGroupsLanding.
  • Fix debounce issue in LinodeSelectTable.
  • Removed customeValue prop usage in LinodeSelectTable and SelectLinodePanel.

Target release date πŸ—“οΈ

09/02

How to test πŸ§ͺ

Verification steps

(How to verify changes)

  • Checkout the branch and verify there is no regression for all the references of PlacementGroupsLanding, SelectLinodePanel and LinodeSelectTable
  • Verify debounce behavior for PlacementGroupsLanding and LinodeSelectTable

As an Author I have considered πŸ€”

Check all that apply

  • [ ] πŸ‘€ Doing a self review
  • [ ] ❔ Our contribution guidelines
  • [ ] 🀏 Splitting feature into small PRs
  • [ ] βž• Adding a changeset
  • [ ] πŸ§ͺ Providing/Improving test coverage
  • [ ] πŸ” Removing all sensitive information from the code and PR description
  • [ ] 🚩 Using a feature flag to protect the release
  • [ ] πŸ‘£ Providing comprehensive reproduction steps
  • [ ] πŸ“‘ Providing or updating our documentation
  • [ ] πŸ•› Scheduling a pair reviewing session
  • [ ] πŸ“± Providing mobile support
  • [ ] β™Ώ Providing accessibility support

cpathipa avatar Aug 22 '24 16:08 cpathipa

Coverage Report: βœ…
Base Coverage: 86.15%
Current Coverage: 86.15%

github-actions[bot] avatar Aug 22 '24 16:08 github-actions[bot]

@hkhalil-akamai @mjac0bs Great feedback. @mjac0bs Good call, Actually, the root cause of the issue is having onSearch as a dependency in the useEffect, which is causing unwanted re-triggers. Removing it from the dependencies would prevent this from happening in the future. Also, I don't see the use case for having it in the dependency list. The only concern I see with renaming onSearch to onChange is that we might override the onChange prop of TextFieldPropsOverrides. In my opinion, onSearch is a wrapper for onChange in DebouncedSearchTextField. Let me know your thoughtsβ€”I'm open to changing the prop name to onChange as well.

cpathipa avatar Aug 28 '24 20:08 cpathipa

@mjac0bs Addressed the above feedback in the commit 9bb33ae Changes include

  • Memoized the debounced onChange handler to prevent unnecessary re-creations.
  • Improved the useEffect hook to handle empty values and reduce unnecessary state updates.
  • removed redundant logic of the clear icon from the instanced of DebouncedSearchTextField.

The onSearch and value props are now required to ensure that the component can function correctly. The onSearch prop is essential for handling search. The value prop is necessary for managing the controlled input state. Making these props required enforces proper usage and helps prevent unintended behavior if they are accidentally omitted.

cpathipa avatar Sep 03 '24 15:09 cpathipa

@mjac0bs fixed the console error in Longview a1b2e8f

cpathipa avatar Sep 03 '24 18:09 cpathipa