Make Modal disappear OnScroll in React.js-CodePudding

I was asked to make a modal disappear onScroll. Which I achieved but upon code review I was told that the code should only run once and that I shouldn't add event listeners. I'm beyond confused, Any help would be appreciated.

useEffect(() => {
    const handleScroll = () => {
      return selectEntryGroup(undefined); //setting selectEntryGroup to undefined will kill the modal
    };

    window.addEventListener('scroll', handleScroll);
    return () => {
      window.removeEventListener('scroll', handleScroll); //cleanup function
    };
  }, []);

When I run this code the modal disappears as expected but when I add a console log inside my handleScroll function it does indeed run even when no modal is present. This I understand.

My thinking is that if I add a condition checking if the modal is open then I could have the function only run once.

However, I created a piece of state (popup, setPopup)that defaults to "false", that is plugged in to the onClick that makes the modal open up. When I console log popup it changes to true based on the onClick but the modal doesn't disappear.

Here's my code with the condition.

 useEffect(() => {
    if(popup === true){
      
    const handleScroll = () => {
      selectEntryGroup(undefined);
      console.log("FIRED!")
      return
    };
    
    window.addEventListener('scroll', handleScroll);
    return () => {
      window.removeEventListener('scroll', handleScroll); //cleanup function
    };
   }else{
    console.log("Didn't Work!")
   }
  }, []);

Any ideas?

CodePudding user response：

Your first iteration of the useEffect looks great. Perfectly implemented and used.

Your code with the condition makes it look like you don't fully understand what is happening with the useEffect. Your useEffect runs once during the initial render. It checks whether or not popup is true, it is not, and therefore none of the rest of the code runs. Even if popup later changes to true, this useEffect has already run its course during the initial render and no code is run.

Instead of checking if popup is true before setting the event listeners, you should be checking if popup is true upon the user scrolling.

useEffect(() => {
    const handleScroll = () => {
      if(!popup) return; // Popup is not open, therefore return

      selectEntryGroup(undefined);
    };

    window.addEventListener('scroll', handleScroll);
    return () => {
      window.removeEventListener('scroll', handleScroll); //cleanup function
    };
  }, []);

CodePudding user response：

I don't see a problem with your current code. However, you could remove the event listener after the event is fired for the first time.

 useEffect(() => {

    const handleScroll = () => {
      selectEntryGroup(undefined);
      removeScrollListener();
    };
    const removeScrollListener = () => window.removeEventListener('scroll', handleScroll);
    
    window.addEventListener('scroll', handleScroll);
      
    return () => {
      removeScrollListener();
    };
  }, []);