Skip to content
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

useElementSize() returns 0 width/height if initially rendered to the DOM within a parent with display: none #115

Closed
Jake-Lippert opened this issue Feb 18, 2022 · 10 comments
Labels
bug Something isn't working

Comments

@Jake-Lippert
Copy link

When the element to be sized is within another element that's conditionally hidden/shown via display none/block, the hook can't determine its size once the parent is visible. If you resize the browser, then the size will be correct at the time of resize. However, if you hide the parent, the element will still report its size as being present.

For my use case, having a non-zero size reported once the parent is hidden isn't a problem, though reporting a size of 0x0 (until browser resize) is a problem. I've forked your hook demo so that it reproduces the issue.

https://codesandbox.io/s/usehooks-ts-react-component-template-forked-tzw9yv?file=/src/App.tsx
Possibly related to #32

@TunA-Kai
Copy link
Contributor

TunA-Kai commented Feb 19, 2022

A quick fix could be adding the key prop to the element with your ref like this:

<div style={{ display: isVisible ? "block" : "none" }}>
    <div
      key={String(isVisible)}
      ref={squareRef}
      style={{
        width: "50%",
        paddingTop: "50%",
        backgroundColor: "aquamarine",
        margin: "auto"
      }}
    />
</div>

But I'm not the big fan of it so I fix the problem by exposing handlesize function from the custom hook and call it when state(isVisible) change.

Here is demo: https://codesandbox.io/s/80cvrv

@juliencrn
Copy link
Owner

Hi guys, thanks for sending feedback :)

In my opinion, that isn't a bug, it's the normal css/js behavior.
A DOM Element that's display: none; (and its children) cannot either be measured or animated (CSS Animate).

Here, a vanilla JS example:

Screenshot 2022-02-20 at 22 58 07


If you want a smooth show/hide animation, this hook is not the right tool, instead, you could use a library like React transition group or try something like that:

expand code

Example code from a personal project (demo)

import { useEffect, useRef, useState } from "react"
import { ChevronDownIcon } from "./Icons"
import cn from "classnames"

interface PropTypes {
  title: string
  content: JSX.Element
  open?: boolean
}

const Collapse = ({ open, title, content }: PropTypes) => {
  const [isOpen, setIsOpen] = useState(open)
  const [height, setHeight] = useState(0)
  const ref = useRef<HTMLDivElement>(null)

  const toggleOpen = () => {
    setIsOpen((prev) => !prev)
  }

  useEffect(() => {
    if (isOpen && !!ref.current) {
      setHeight(ref.current.getBoundingClientRect().height)
    } else {
      setHeight(0)
    }
  }, [isOpen])

  return (
    <article className="my-6 md:my-12">
      <h3
        onClick={toggleOpen}
        className="h3 cursor-pointer font-black mb-2 flex justify-between"
      >
        {title}
        <span
          className={cn(
            isOpen ? "rotate-180" : "",
            "transition-transform duration-200"
          )}
        >
          <ChevronDownIcon />
        </span>
      </h3>
      <div
        className="overflow-hidden"
        style={{
          transition: "height 0.2s ease-in-out",
          height,
        }}
      >
        <div ref={ref}>{isOpen && <div>{content}</div>}</div>
      </div>
    </article>
  )
}

export default Collapse

@Jake-Lippert
Copy link
Author

I didn't end up going with your solution, but it did help give my mind a kick toward another solution. :)

I ended up passing additionalDependencies?: React.DependencyList into the hook and combining that into the dependencies used by useLayoutEffect.

@juliencrn
Copy link
Owner

Glad to help :)

@TunA-Kai
Copy link
Contributor

TunA-Kai commented Feb 22, 2022

This hook really bugs me so I try to fix the problem without eposing from or passing in the hook anything more.

I read the old version of this hook and I have to say using ref callback is really creative for me. But it comes with a cost

React will call the ref callback with the DOM element when the component mounts, and call it with null when it unmounts.

That means the ref only be updated when the component unmount. That's why if we change display from block to none, the component doesn't unmount and the hook doesn't update the size state.

So I comeback to the old way of using useRef. We have the size state always hold the old value of ref.current.offsetWidth and ref.current.offseHeight, and the lastest value is always in the ref itself. That means we just need to compare those things to decide if we need to update size or not.

Here is the implementation: codesandbox

function useElementSize<T extends HTMLElement>() {
  // change to useRef
  const ref = useRef<T>(null);
  const [size, setSize] = useState<Size>({
    width: 0,
    height: 0
  });

  const handleSize = useCallback(() => {
    setSize({
      width: ref.current?.offsetWidth ?? 0,
      height: ref.current?.offsetWidth ?? 0
    });
  }, []);

  useEventListener("resize", handleSize);

  useEffect(() => {
    if (
      size.width !== ref.current?.offsetWidth ||
      size.height !== ref.current?.offsetHeight
    )
      handleSize();
  });

  return [ref, size] as const;
}

So what do you think about this?

By the way, if we use callback ref then this is what I think the dependencies list should look like:

  const handleSize = useCallback(() => {
    setSize({
      width: ref?.offsetWidth || 0,
      height: ref?.offsetHeight || 0
    });
  }, [ref]);

  useLayoutEffect(() => {
    handleSize();
  }, [handleSize]);

@juliencrn juliencrn reopened this Feb 22, 2022
@juliencrn
Copy link
Owner

Hi @TunA-Kai,
Indeed, your implementation fixes the bug. I'm open to improve it.
If we'll do it, I think we can leave the const ref = useRef<T>(null); outside the hook to be more composable and flexible (& easier to test), and thus, update the return statement. The new function signature could be like:

import { RefObject } from 'react'
interface Size { width: number, height: number }

function useElementSize<T extends HTMLElement = HTMLDivElement>(ref: RefObject<T>): Size

Would you open an PR?

Note:

  • We have to update the demo and the tests following the new function signature
  • The new function signature will create a breaking-change in the API, but it's ok, I'm thinking about release a v3 soon.

@TunA-Kai
Copy link
Contributor

TunA-Kai commented Feb 28, 2022

Hi @juliencrn,
Totally agree with your opinion. But I don't know how to test a custom hook so I can't make any changes to the test file. I could open an PR to update the demo and the hook itself though.

Base on your idea, I have changed the implementation of the hook again:

  • Move useRef outside of custom hook. But I don't give a default type parameter to HTMLDivElement because I think specific about the type of ref when using useRef is a good practice.
  • Remove useCallback since useEventListener don't need a memoize function anymore
  • Move the comparison of the old size and the new one from useEffect to handlerSize function since it helps reduce the time the component is re-rendered because of resize window event. You can test that by compare how many times the component re-rendered when the size is 0 and we resize the window between the old and new implementation.
  • resize event is one of the events that people might want to debounce so I also add a helper debounce function and delayWindowResizeUpdate parameter to the hook so user can choose to debounce that event or not. The default value is 0.
Code details codesandbox
function useElementSize<T extends HTMLElement>(
  ref: RefObject<T>,
  delayWindowResizeUpdate: number = 0,
): Size {
  // console.log("custom hook run");
  const [size, setSize] = useState<Size>({
    width: 0,
    height: 0,
  })

  const handleSize = () => {
    const newWidth = ref.current?.offsetWidth
    const newHeight = ref.current?.offsetHeight

    if (size.width !== newWidth || size.height !== newHeight)
      setSize({
        width: newWidth ?? 0,
        height: newHeight ?? 0,
      })
  }

  useEventListener('resize', debounce(handleSize, delayWindowResizeUpdate))

  useEffect(() => {
    handleSize()
  })

  return size
}

function debounce<Callback extends (...args: Array<unknown>) => void>(fn: Callback, delay: number) {
  let timer: ReturnType<typeof setTimeout> | null = null
  return (...args: Parameters<Callback>) => {
    if (timer) clearTimeout(timer)
    timer = setTimeout(() => {
      fn(...args)
    }, delay)
  }
}

@juliencrn
Copy link
Owner

Nice @TunA-Kai, thanks for all the details! 💯

I will do it, I think you can't push a PR properly without the tests, husky will block you, and if it doesn't block, it will include breaking tests in the rebase and merge.

I will ask you a code review soon :)

@PremExtensi
Copy link

Hi @juliencrn any update on the release date?

@juliencrn juliencrn added the bug Something isn't working label Jan 8, 2024
@juliencrn
Copy link
Owner

Hi, useElementSize has been deprecated and should be replaced by useResizeObserver.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants