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

Enhanced mobile navbar for improved user experience #960

Conversation

anjankarmakar10
Copy link

I notice some issues with your mobile navbar. When a user tries to navigate by clicking a nav item, the navbar won't close automatically, which is not a good user experience.

Changes and Improvements:

  1. Use of useCallback:
    In the new code, you've introduced the useCallback hook to create the handleToggle function. The useCallback hook memoizes the function, which means that the function reference remains the same across re-renders as long as its dependencies (in this case, an empty dependency array) remain unchanged. This can be beneficial in terms of performance because it prevents unnecessary re-renders of components that use this function. In the previous code, a new function was created on every render, which could lead to unnecessary re-renders of components that used this function.

  2. Component Composition:
    You've also introduced a new NavItem component that encapsulates the behavior of a navigation item. This component wraps the Link component and attaches the handleToggle function to the onClick event. This ensures that clicking on a navigation item not only navigates to the desired route but also triggers the closing of the navigation menu. This encapsulation improves code readability and reusability, as the behavior is abstracted into a separate component.

  3. Consistency and Maintainability:
    By centralizing the behavior of toggling the navigation menu (isOpen state) within the handleToggle function, you've eliminated code duplication. This not only makes your codebase more consistent but also easier to maintain. If you ever need to update the behavior of toggling the navigation menu, you can do so in a single place.

  4. Enhanced User Experience:
    The primary improvement you've made is related to user experience. In the previous code, clicking on a navigation item did not automatically close the navigation menu, which could be confusing and cumbersome for users. In the new code, clicking on a navigation item triggers both navigation to the desired route and the closing of the navigation menu, providing a smoother and more intuitive user experience.

By utilizing useCallback to optimize function references, introducing component composition for encapsulating behavior, and improving the user experience by addressing the issue with navigation menu behavior, you've made significant enhancements to the mobile navigation functionality in your open-source project.

@anjankarmakar10 anjankarmakar10 requested a review from a team as a code owner August 10, 2023 06:10
@netlify
Copy link

netlify bot commented Aug 10, 2023

👷 Deploy Preview for virtual-coffee-io processing.

Name Link
🔨 Latest commit 0707a08
🔍 Latest deploy log https://app.netlify.com/sites/virtual-coffee-io/deploys/64d47f6b94a2e100089bf754

@BekahHW
Copy link
Member

BekahHW commented Aug 24, 2023

Hey @anjankarmakar10 have you checked out this issue #823 and the accompanying PR? Does that impact the work you've done?

@anjankarmakar10
Copy link
Author

Hey @BekahHW The only change was the navbar component. So, it should not impact the rest of the project. However, I would recommend testing or reviewing the changes before merging them into the master.

Let me know if you have any questions.

Copy link
Member

@danieltott danieltott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @anjankarmakar10 - thanks for the PR!

A couple notes:

  • handleToggle is memoized, but NavItem is not, so we're getting re-renders every time regardless, so we're not getting any real benefits there.
  • I think the NavItem component is an over-optimization. I like the idea of using useCallback for handleToggle but I don't think we need to introduce another new component.

How about something like this instead:

const navItemProps = useMemo(()=> ({
  onClick: ()=>setIsOpen((open) => !open),
  className: "nav-link"
}), []);

And then in the list items:

<Link
  to="/about"
  {...navItemProps}
>
  About
</Link>

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

Successfully merging this pull request may close these issues.

None yet

3 participants