You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
**issue (complexity):** Consider simplifying the code by avoiding the custom hook and keeping the body scroll control logic within the component.
The new code introduces a custom hook useMobileNav, which adds an additional layer of abstraction and complexity. While custom hooks can be useful for reusability, they make the code harder to understand at a glance. Additionally, the new code adds more props (key, speed, reverse) to the UseAnimations component, increasing the cognitive load. The removal of the body scroll control logic from the component is also a concern, as it is a crucial piece of functionality that is not immediately visible.
Here is a less complex way to make the code change while keeping the body scroll control and avoiding the custom hook:
'use client';import{useEffect}from'react';import{useToggle}from'react-use';importUseAnimationsfrom'react-useanimations';importmenu4from'react-useanimations/lib/menu4';import{NavLinks}from'./nav-links';import{Transition}from'@/components/shared/transition';import{AnimatePresence}from'framer-motion';import{LangProp}from'@/types/routing.type';exportfunctionMobileNav({ lang, dict }: MobileNavProps){const[open,toggleOpen]=useToggle(false);// control the body scrolluseEffect(()=>{document.body.style.overflow=open ? 'hidden' : 'auto';return()=>{document.body.style.overflow='auto';};},[open]);return(<div><UseAnimationsonClick={toggleOpen}strokeColor="white"animation={menu4}wrapperStyle={{marginRight: '-10px'}}size={56}speed={2.42}// Added new propreverse={open}// Added new prop/><AnimatePresence>{open ? (<Transitionduration={0.3}><divclassName="mobile-nav"><NavLinksmobilelang={lang}dict={dict}toggleOpen={toggleOpen}// Keeping this prop for consistency/></div></Transition>) : null}</AnimatePresence></div>);}exportfunctionMobileNavLoader(){return(<UseAnimationsstrokeColor="white"animation={menu4}/>);}
This version keeps the body scroll control logic within the component, avoids the additional complexity of a custom hook, and maintains the new props for UseAnimations.
This is a good feedback. @Bran18, when checking the details for the UI/UX recent changes, can you take a look into this? This can be done later. Let me know what you think!
AndlerRL
changed the title
**Issue (Complexity):** Simplifying the useMobileNav hook.
Issue (Complexity): Simplifying the useMobileNav hook.
Aug 6, 2024
The new code introduces a custom hook
useMobileNav
, which adds an additional layer of abstraction and complexity. While custom hooks can be useful for reusability, they make the code harder to understand at a glance. Additionally, the new code adds more props (key
,speed
,reverse
) to theUseAnimations
component, increasing the cognitive load. The removal of the body scroll control logic from the component is also a concern, as it is a crucial piece of functionality that is not immediately visible.Here is a less complex way to make the code change while keeping the body scroll control and avoiding the custom hook:
This version keeps the body scroll control logic within the component, avoids the additional complexity of a custom hook, and maintains the new props for
UseAnimations
.Originally posted by @sourcery-ai[bot] in #302 (comment)
The text was updated successfully, but these errors were encountered: