minop1205 / react-dnd-treeview

A draggable / droppable React-based treeview component. You can use render props to create each node freely.
MIT License
519 stars 69 forks source link

feat: support expand animation #169

Closed percy507 closed 1 year ago

percy507 commented 1 year ago

167

minop1205 commented 1 year ago

@percy507

Thank you for your cooperation. Please allow me time for review as there are many changes to be made.

minop1205 commented 1 year ago

@percy507 I confirmed. Thank you for adding this wonderful feature!

Generally there are no problems, but there was one issue that concerned me, and I would like to suggest what it is and how to correct it.

In AnimateHeight.tsx, when isVisible transitions from false to true, it looks only the fade-in animation appears to be active because the child nodes are not masked.

It feels especially strange when animating the element at the bottom of the tree.

https://user-images.githubusercontent.com/3772820/204113572-9d7d045c-0a41-4201-b514-0d5a55c77f45.mp4

This problem can be avoided by changing the style specification in AnimateHeight.tsx as follows and always masking the child nodes.

// before
style={isVisible ? {} : {overflow: "hidden"}}

// after
style={{overflow: "hidden"}}

https://user-images.githubusercontent.com/3772820/204113609-80c9d57e-1b77-49ea-b5de-bf60bd61da1f.mp4

However, this approach causes other problems.

If you have sorting enabled using placeholders, it is possible that some of the placeholders may be masked, as shown in the video below. It is difficult to see in the video, but some of the placeholder lines are masked and thin.

https://user-images.githubusercontent.com/3772820/204113624-cfe97d94-e2a4-4313-b028-7e5fd4e25449.mp4

To avoid both problems, I think it would be necessary to implement a mask that is always applied during animation and otherwise follows isVisible.

Specifically, I propose the following implementation.

export function AnimateHeight(props: AnimateHeightProps) {
  const {
    isVisible,
    ease = "easeIn",
    duration,
    variants = {
      open: { opacity: 1, height: "auto" },
      close: { opacity: 0, height: 0 },
    },
    children,
  } = props;
  const [ref, { height }] = useMeasure({ polyfill: ResizeObserver });
  const [animating, setAnimating] = useState(false);
  const onAnimationStart = () => {
    setAnimating(true);
  };
  const onAnimationComplete = () => {
    setAnimating(false);
  };
  return (
    <motion.div
      //style={isVisible ? {} : { overflow: "hidden" }}
      style={isVisible && !animating ? {} : { overflow: "hidden" }}
      onAnimationComplete={onAnimationComplete}
      onAnimationStart={onAnimationStart}
      initial={isVisible ? "open" : "close"}
      animate={isVisible ? "open" : "close"}
      inherit={false}
      variants={variants}
      transition={{ ease, duration: computeDuration(height, duration) }}
    >
      <div ref={ref}>{children}</div>
    </motion.div>
  );
}

Please consider. Best regards.

percy507 commented 1 year ago

@minop1205 I think it's fine. As you wish, I have make a commit.

BTW, there is a potential issue, when there are lots of nodes (in my test, it's 700+), openAll will be stuck for a moment. I think it's a performance issue and hard to resolve.

minop1205 commented 1 year ago

@percy507 Confirmed. Thank you!

BTW, there is a potential issue, when there are lots of nodes (in my test, it's 700+), openAll will be stuck for a moment. I think it's a performance issue and hard to resolve.

Yes, it is.

If enableAnimateExpand is true, there is a performance issue because all hidden nodes are included in the rendering.

Performance tuning should be done if possible, but that is a separate task.