Hi, there are a couple of things here that could lead to some Jank so I thought I'd give you some pointers, I hope that's alright 😊.
You're listener function is outside of the useEffect it's used in. This means it is remade on every draw, this isn't a huge problem with onclick events and stuff but when it comes to using them in a useEffect it means the the useEffect runs every draw (as does its return functions).
In this instance you've not got a dependencies array on your useEffect anyway meaning it is ran every draw. It would be better to have an empty array as the dependencies as that means it will only run on mount and then return on unmount.
You're calling getBoundingClientRect on each scroll that's quite a heavy function to be calling so often. It's worth remembering that window already knows how far it's scrolled so we can just get it off that.
A couple of extra points I'd add in are the scroll event listener is heavy as it is so I'd include a context with your hook that a dev can use to use the same listener else where in the app and on the same note you could make the function more generic by giving not just the current x and y but also the last x and y then you don't need to include the direction as it's a simple calculation the dev can include else where.
I think with just those couple of changes you could change the hook from a great concept with a good execution to a great concept with a great execution. I've made a quick demo with my changes for you to have a look at 😊
Ooh yes! Adding event listeners like that is a really really good example of a memory leak. I once read an article where soundcloud developers said that their app was getting slower after a lot of use. It was because they had the same issue. Attaching events without removing them.
Also, the Sandbox you shared, there is an empty folder called useScrollListener. The import names are a little confusing too. I also want to know why you didn't use a useRef hook instead of using the className attribute?
I've fixed the names, I changed the name part way through making the demo so I guess it got confused and kept made a new version rather than renaming.
Controlling the classes like this means you know for sure what classes you element has if you start adding and removing them with JS you have to keep track of all the classes and make sure you don't remove any by accident, or even leave any on the element when you don't mean to.
I agree with all the things that you have told me. The dependency things is a mistake on my side. But Other things that you added a context and that getBoundingClientRect function is a heavy function, I was not aware of those.
I get it that we can calculate the direction when we need and don't really have to do that in custom hook itself.
creates some flickering if the navbar has a transparent background and a blurring of the backdrop. This is quite common for navbars IMO, so it's worth pointing out the alternative here, which doesn't interfere with the blurring:
nav {
/* ... */
transition: top 150ms ease-in-out;
}
nav.nav-bar--hidden {
top: -100%;
}
For further actions, you may consider blocking this person and/or reporting abuse
We're a place where coders share, stay up-to-date and grow their careers.
Hi, there are a couple of things here that could lead to some Jank so I thought I'd give you some pointers, I hope that's alright 😊.
You're
listenerfunction is outside of the useEffect it's used in. This means it is remade on every draw, this isn't a huge problem with onclick events and stuff but when it comes to using them in a useEffect it means the the useEffect runs every draw (as does its return functions).In this instance you've not got a dependencies array on your useEffect anyway meaning it is ran every draw. It would be better to have an empty array as the dependencies as that means it will only run on mount and then return on unmount.
You're calling
getBoundingClientRecton each scroll that's quite a heavy function to be calling so often. It's worth remembering that window already knows how far it's scrolled so we can just get it off that.A couple of extra points I'd add in are the scroll event listener is heavy as it is so I'd include a context with your hook that a dev can use to use the same listener else where in the app and on the same note you could make the function more generic by giving not just the current x and y but also the last x and y then you don't need to include the direction as it's a simple calculation the dev can include else where.
I think with just those couple of changes you could change the hook from a great concept with a good execution to a great concept with a great execution. I've made a quick demo with my changes for you to have a look at 😊
For those who are using styled components.
dev-to-uploads.s3.amazonaws.com/up...
Ooh yes! Adding event listeners like that is a really really good example of a memory leak. I once read an article where soundcloud developers said that their app was getting slower after a lot of use. It was because they had the same issue. Attaching events without removing them.
Also, the Sandbox you shared, there is an empty folder called useScrollListener. The import names are a little confusing too. I also want to know why you didn't use a useRef hook instead of using the className attribute?
I've fixed the names, I changed the name part way through making the demo so I guess it got confused and kept made a new version rather than renaming.
Controlling the classes like this means you know for sure what classes you element has if you start adding and removing them with JS you have to keep track of all the classes and make sure you don't remove any by accident, or even leave any on the element when you don't mean to.
I agree with all the things that you have told me. The
dependencythings is a mistake on my side. But Other things that you added a context and thatgetBoundingClientRectfunction is a heavy function, I was not aware of those.I get it that we can calculate the direction when we need and don't really have to do that in custom hook itself.
Thanks for taking your time
Highly Appreciate it !!
I think you do great code reviews, Andrew !
I will be changing the article accordingly.
@link2twenty Great analysis and improvements!
If I may add two cents - the approach with
nav.nav-bar--hidden {
transform: translateY(-100%);
}
creates some flickering if the navbar has a transparent background and a blurring of the backdrop. This is quite common for navbars IMO, so it's worth pointing out the alternative here, which doesn't interfere with the blurring:
nav {
/* ... */
transition: top 150ms ease-in-out;
}
nav.nav-bar--hidden {
top: -100%;
}