DEV Community

Mingming Ma
Mingming Ma

Posted on

My PRs to ant-design: PR2

In my last post, I shared my experience about my first pull request to ant-design. In this post, I would like to continue writing about my second PR. Hope it's helpful to you.

Issue

The issue was opened by li-jia-nan. This issue was about the component Affix. Users can wrap Affix around another component to make it stick the viewport. And it supports setting a target element:
Affix Target

The issue was that it didn't work as expected after some changes, which means that when the user scrolls up, the blue button would not fixed at the top of container, See:
Affix Issue

Debug process

I spent lots of time checking the source code ant-design/components/affix/index.tsx. What I did was adding lots of logs of each function to familiar with the logic of calling relations. Although it was a bit boring and frustrating before finding the root cause, thanks to the help of logs, I finally realized that the logic of this code should be fixed:

Before:

  const addListeners = () => {
    const listenerTarget = targetFunc?.();
    TRIGGER_EVENTS.forEach((eventName) => {
      if (prevListener.current) {
        prevTarget.current?.removeEventListener(eventName, prevListener.current);
      }
      listenerTarget?.addEventListener(eventName, lazyUpdatePosition);
    });
    prevTarget.current = listenerTarget;
    prevListener.current = lazyUpdatePosition;
  };
Enter fullscreen mode Exit fullscreen mode

After:

  const addListeners = () => {
    const listenerTarget = targetFunc?.();

    // added
    if (!listenerTarget) {
      return;
    }

    TRIGGER_EVENTS.forEach((eventName) => {
      if (prevListener.current) {
        prevTarget.current?.removeEventListener(eventName, prevListener.current);
      }
      listenerTarget?.addEventListener(eventName, lazyUpdatePosition);
    });
    prevTarget.current = listenerTarget;
    prevListener.current = lazyUpdatePosition;
  };
Enter fullscreen mode Exit fullscreen mode

In before code, when listenerTarget is null, the prevTarget can still do removeEventListener, so the prevTarget element will not listen to the scroll event anymore. This sounds a bit funny: there was no scrolling EventListener, yet the button started scrolling instead. In fact, that EventListener is used to monitor the relative position and add CSS position: fixed to make this button fixed. What I did was add a check on the listenerTarget, if it is null, the function addListeners just returns, so the prevTarget remains unaffected.

Reflection

The key to fixing this issue is patience, print functions' passing values little by little to understand the whole picture. I admit that this approach might be a bit clumsy, but it's still a way of tackling the problem, isn't it? After all, in the world of coding, sometimes a little clumsiness is just the first step towards mastering a new skill.

Top comments (0)