skip to Main Content

Here is a code of a search filter component where user can find products by typing letters inside the input field. I wonder which improvements can i add. I was thinking of removing some logic into custom hook, but don’t consider it necessary since I use it only once.

p.s. I know that it would be better to use sth like Stack Exchange for such kind of question, but I wanna get the answer.

Component:

export const SearchItems = ({
  openBackdrop,
  setOpenBackDrop,
}: SearchItemsProps) => {
  const theme = useTheme();
  const [searchQ, setSearchQ] = useState<string>("");
  const [isScrollDisabled, setIsScrollDisabled] = useState<boolean>(false);
  const [products, setProducts] = useState<Product[]>([]);
  const [productsByQ, setProductsByQ] = useState<Product[]>([]);

  useEffect(() => {
    const getProducts = async () => {
      try {
        const productsRef = await getDocs(collection(db, "products"));
        const products = productsRef.docs.map((doc) => ({
          ...doc.data(),
        }));
        const flattenData: Product[] = products.flatMap(
          (product) => product.data
        );
        setProducts(flattenData);
      } catch (error: any) {
        console.log(error.message);
      }
    };

    getProducts();
  }, []);

  const handleFilterProductsByQ = useMemo(
    () =>
      debounce((searchQ: string) => {
        const filteredData = products.filter((product) =>
          product.name.toLowerCase().includes(searchQ.toLowerCase())
        );
        setProductsByQ(filteredData);
      }, 300),
    [products]
  );

  useEffect(() => {
    handleFilterProductsByQ(searchQ);
  }, [searchQ]);

  useEffect(() => {
    isScrollDisabled
      ? (document.body.style.overflow = "hidden")
      : (document.body.style.overflow = "auto");

    return () => {
      document.body.style.overflow = "auto";
    };
  }, [isScrollDisabled]);

  const handleInputFocus = () => {
    window.scrollTo({ top: 0, behavior: "auto" });
    setIsScrollDisabled(true);
    setOpenBackDrop(true);
  };

  const handleOutsideClick = () => {
    setOpenBackDrop(false);
    setIsScrollDisabled(false);
    setSearchQ("");
  };

  return (
    <OutsideClickHandler onOutsideClick={handleOutsideClick}>
      <Box position="relative">
        <OutlinedInput
          type="text"
          placeholder="Search Item…"
          value={searchQ}
          onChange={(e) => setSearchQ(e.target.value)}
          onFocus={handleInputFocus}
          startAdornment={
            <InputAdornment
              position="start"
              sx={{
                color: theme.palette.primary.main,
                fontSize: "1.2rem",
              }}
            >
              <FiSearch />
            </InputAdornment>
          }
          endAdornment={
            <InputAdornment position="end">
              <IconButton
                edge="end"
                onClick={() => setSearchQ("")}
                sx={{
                  color: theme.palette.secondary.light,
                  cursor: "pointer",
                  fontSize: "0.1rem",
                }}
              >
                {searchQ !== "" && <CloseIcon />}
              </IconButton>
            </InputAdornment>
          }
          sx={{
            borderRadius: "0.8rem",
            border: "none",
            backgroundColor:
              openBackdrop === true ? "#f9fafb" : theme.palette.primary.light,
            zIndex: openBackdrop === true ? 9999 : "none",
            "& .MuiOutlinedInput-notchedOutline": {
              border: "none",
            },
          }}
        />
        {searchQ.length !== 0 && (
          <SearchResult searchQ={searchQ} productsByQ={productsByQ} />
        )}
      </Box>
    </OutsideClickHandler>
  );
};

2

Answers


  1. You don’t need this
    const [productsByQ, setProductsByQ] = useState<Product[]>([]); and the whole logic around it. So you already have a state that holds the products and a state for the search term.

    Every time your search term changes, it triggers a component re-render thus you can directly use your search term and products like this:

    {
      searchQ.length !== 0 && (
        <SearchResult
          searchQ={searchQ}
          productsByQ={products.filter((product) =>
            product.name.toLowerCase().includes(searchQ.toLowerCase())
          )}
        />
      )
    }
    

    Then you can move your debounce logic for the search term thus you won’t be filtering by every key stroke

    Login or Signup to reply.
  2. I’m not a TypeScript developer, so the given code is simply using JavaScript or the types you already had present. But if I’m not mistaken most types in these scenario’s can be inferred by TypeScript. If not you’ll need to add them yourself. With that out of the way let’s get to the first point.

    debounce makes the component way more complex than it needs to be. If possible I would look into useDeferredValue.

    Let me give an example using your given code:

    const [searchQ, setSearchQ] = useState<string>("");
    const deferredSearchQ = useDeferredValue(searchQ);
    const [products, setProducts] = useState<Product[]>([]);
    const productsByQ = useMemo(() => (
      products.filter((product) => (
        product.name.toLowerCase().includes(deferredSearchQ.toLowerCase())
      ))
    ), [products, deferredSearchQ]);
    
    // during render
    {deferredSearchQ.length && (
      <SearchResult searchQ={deferredSearchQ} productsByQ={productsByQ} />
    )}
    

    This would remove the need for the following code:

    // this is now a useMemo
    const [productsByQ, setProductsByQ] = useState<Product[]>([]);
    
    const handleFilterProductsByQ = useMemo(
      () =>
        debounce((searchQ: string) => {
          const filteredData = products.filter((product) =>
            product.name.toLowerCase().includes(searchQ.toLowerCase())
          );
          setProductsByQ(filteredData);
        }, 300),
      [products]
    );
    
    useEffect(() => {
      handleFilterProductsByQ(searchQ);
    }, [searchQ]);
    

    The above already shrinks the component quite a bit. If you need more speed during the first render, or renders where the search query is empty you could short circuit the useMemo and skip the .filter() if the query is empty. This does grow the complexity, but is still simpler than the logic you had before.

    const productsByQ = useMemo(() => {
      // skip .filter if the search query is empty (return all products)
      if (!deferredSearchQ.length) return products;
      return products.filter((product) => (
        product.name.toLowerCase().includes(deferredSearchQ.toLowerCase())
      ));
    }, [products, deferredSearchQ]);
    

    I would also suggest to use memo (if you haven’t already) to memoize SearchResult. This lets you skip re-rendering the SearchResult component when its props are unchanged. This can happen quite a lot when used in combination with useDeferredValue.


    Another thing to note is that you currently use the conditional (ternary) operator … ? … : … as a substitution for a normal if/else statement.

    useEffect(() => {
      isScrollDisabled
        ? (document.body.style.overflow = "hidden")
        : (document.body.style.overflow = "auto");
    
      return () => {
        document.body.style.overflow = "auto";
      };
    }, [isScrollDisabled]);
    

    I would personally use the fact that the conditional (ternary) operator is an expression (it returns a value) and is not just a statement. The above could be changed into:

    useEffect(() => {
      document.body.style.overflow = isScrollDisabled ? "hidden" : "auto";
      return () => document.body.style.overflow = "auto";
    }, [isScrollDisabled]);
    

    Since the clean-up function only contains a single statement that could also function as an expression, you can remove the block delimiters of the clean-up function and keep the expression on the same line. This will change the return type of the clean-up function, but since the return value is not used by React that shouldn’t matter.


    Finally, most of the time someVariable === true (or someVariable === false) is an anti-pattern when used inside an if-statement or conditional (ternary) operator. This can often be replaced by just someVariable (or !soveVariable) to check for a truthy/falsy value.

    In your scenario:

    openBackdrop === true ? "#f9fafb" : theme.palette.primary.light
    // could be replaced with
    openBackdrop ? "#f9fafb" : theme.palette.primary.light
    
    openBackdrop === true ? 9999 : "none"
    // could be replaced with
    openBackdrop ? 9999 : "none"
    
    Login or Signup to reply.
Please signup or login to give your own answer.
Back To Top
Search