Javascript Question posted in
A very good W3school tutorial can be found here.

Javascript – Is it bad practice to make useEffect respond to an user action like this?

In the following code, I wanted to put all the collapsible logic into the Collapsible component. However, the collapse is being triggered by a button that’s outside of Collapsible. So I made Collapsible to react to that button’s onClick. For that, I had to use useEffect:

// JS:

const [collapsed, setCollapsed] = useState(true);

// JSX:

<div className="flex w-full justify-between">
  <Heading size="sm" className="shrink-0">
    Tags
  </Heading>
  <Button
    variant="link"
    onClick={() => setCollapsed(!collapsed)}
    className="md:hidden"
  >
    {collapsed ? (
      <IconChevronDown size={20} />
    ) : (
      <IconChevronUp size={20} />
    )}
  </Button>
</div>
<Collapsible collapsed={collapsed} className="md:hidden">
  <div className="mt-4 w-full">{renderedTags}</div>
</Collapsible>

Inside Collapsible, I have this:

useEffect(() => {
  if (collapsed) {
    setMaxHeight('0px');
  } else if (contentRef.current) {
    setMaxHeight(`${contentRef.current.scrollHeight}px`);
  }
}, [collapsed]);

Do you think using useEffect like this to react to user input is bad practice? To prevent that, I could create a useCollapsible hook, but that would separate the collapsible logic from the Collapsible component.

Tags:

2

Answers


  1. 0

    useEffect can be applied here, but React provides a more efficient mechanism for reacting to changes in a prop: calling set functions during rendering. When you set state while the component is rendering, React will skip rendering the children and immediately rerender the component after the function returns.

    This could be implemented like so:

    function Collapse({ collapsed /*, other props... */ }) {
    // other hooks...
    const [prevCollapsed, setPrevCollapsed] = useState();
    if (collapsed !== prevCollapsed) {
        setPrevCollapsed(collapsed);
        if (collapsed)
            setMaxHeight('0px');
        else if (contentRef.current)
            setMaxHeight(`${contentRef.current.scrollHeight}px`);
    }
    return "content to render...";
}
    Login or Signup to reply.
  2. 0

    Do you think using useEffect like this to react to user input is bad practice?

    Yes.

    1. You don’t need the useEffect at all.
    const maxHeight = collapsed ? "0px" : contentRef.current ? `${contentRef.current}px` : null;

    Per to the docs:

    When something can be calculated from the existing props or state, don’t put it in state. Instead, calculate it during rendering. This makes your code faster (you avoid the extra “cascading” updates), simpler (you remove some code), and less error-prone (you avoid bugs caused by different state variables getting out of sync with each other).

    1. It will cause a layout shift.

    useEffect is executed after the component completes rendering. This will introduce layout shift in your example, the component will render with the previous value of maxHeight then it will rerender again with the new value. (useLayoutEffect solves this).

    1. Unnecessary re-render.
    Login or Signup to reply.
Please signup or login to give your own answer.

Back To Top
Search