Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
AndlerRL opened this issue Aug 6, 2024 · 1 comment
Open

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

AndlerRL opened this issue Aug 6, 2024 · 1 comment
Assignees

Comments

@AndlerRL
Copy link
Contributor

AndlerRL commented Aug 6, 2024

          **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 #302 (comment)

@AndlerRL
Copy link
Contributor Author

AndlerRL commented Aug 6, 2024

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 AndlerRL changed the title **Issue (Complexity):** Simplifying the useMobileNav hook. Issue (Complexity): Simplifying the useMobileNav hook. Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants