bitcashorg / smartsale

SmartSale streamlines the auction process on EVM
MIT License
11 stars 3 forks source link

Issue (Complexity): Simplifying the useMobileNav hook. #303

Open AndlerRL opened 1 month ago

AndlerRL commented 1 month ago
          **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';
import UseAnimations from 'react-useanimations';
import menu4 from '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';

export function MobileNav({ lang, dict }: MobileNavProps) {
  const [open, toggleOpen] = useToggle(false);

  // control the body scroll
  useEffect(() => {
    document.body.style.overflow = open ? 'hidden' : 'auto';
    return () => {
      document.body.style.overflow = 'auto';
    };
  }, [open]);

  return (
    <div>
      <UseAnimations
        onClick={toggleOpen}
        strokeColor="white"
        animation={menu4}
        wrapperStyle={{ marginRight: '-10px' }}
        size={56}
        speed={2.42} // Added new prop
        reverse={open} // Added new prop
      />
      <AnimatePresence>
        {open ? (
          <Transition duration={0.3}>
            <div className="mobile-nav">
              <NavLinks
                mobile
                lang={lang}
                dict={dict}
                toggleOpen={toggleOpen} // Keeping this prop for consistency
              />
            </div>
          </Transition>
        ) : null}
      </AnimatePresence>
    </div>
  );
}

export function MobileNavLoader() {
  return (
    <UseAnimations
      strokeColor="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.

_Originally posted by @sourcery-ai[bot] in https://github.com/bitcashorg/smartsale/pull/302#discussion_r1704757604_

AndlerRL commented 1 month ago

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!