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

chore: add mobile menu close, rename navLinks #55

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mikecebul
Copy link

I made the mobile nav menu close when a link was selected for issue #26. The fix was simple by creating a function that sets the state that can be passed down to the child component (main-nav -> mobile-nav). By creating a new prop it made sense to make the items more descriptive to what they really are.

This still leaves the issue of not being able to apply a close animation. The tailwindcss-animate requires a transition component to handle this as shown in their issues. I would do this easily with headlessUI, but it seems like shadcn would prefer to stick with radix. The Radix documentation suggests useTransistion from 'react-spring'. If we're going to do that, then we should probably just use Radix's Navigation Menu primitive.

Let me know which direction you want to go.

This is my first ever pull request and I didn't know if I should name the commit chore or feat. Hopefully you can give me some feedback to improve future pull requests.

@vercel
Copy link

vercel bot commented Dec 7, 2022

Someone is attempting to deploy a commit to a Personal Account owned by @shadcn on Vercel.

@shadcn first needs to authorize it.

@mikecebul mikecebul mentioned this pull request Dec 7, 2022
@vercel
Copy link

vercel bot commented Dec 8, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
taxonomy ✅ Ready (Inspect) Visit Preview Dec 9, 2022 at 10:31AM (UTC)

@shadcn
Copy link
Collaborator

shadcn commented Dec 8, 2022

@mikecebul Thanks for the PR. This is looking good. Man I wish my first PR was as good as this :)

I tried to deploy this but seeing a type error.

Type error: Type '{ items: NavItem[]; }' is not assignable to type 'IntrinsicAttributes & MainNavProps'.

Does this build for you locally?

@mikecebul
Copy link
Author

I updated the naming convention in docs and dashboard to remove the errors. I now realize this is more of an opinionated change and I probably should of just left the naming convention alone.

I can't do a full build because it fails with

Error: Failed to collect page data for /dashboard/settings

I'm guessing because I don't have the env variables.

@mikecebul
Copy link
Author

mikecebul commented Dec 8, 2022

I was digging into the RadixUI docs last night. I'm pretty sure I can get the Navigation Menu Primitive implemented in here too.. That will be the next thing I'll work on.

@mikecebul
Copy link
Author

mikecebul commented Dec 9, 2022

I found it impossible to pass a function to the docs navigation with the component passed as {children}. I decided to make the docs mobile navigation component a conditional in the Main Nav based on the path segment using useSelectedLayoutSegment(). Here's the code section.

{showMobileMenu && (
        <MobileNav items={items} handleShowMobileMenu={handleShowMobileMenu}>
          {segment === "docs" ? (
            <DocsSidebarNav
              items={docsConfig.sidebarNav}
              handleShowMobileMenu={handleShowMobileMenu}
            />
          ) : (
            children
          )}
        </MobileNav>
      )}

Also I reverted the naming convention change of the props "Items" (I changed my mind) and removed the layout file in /docs/[[..slug]] as it wasn't modifying the layout as far as I could tell (and confusing me). I may end up regretting that decision.

This seems to allow 'close on click' with the least change in the code and without using useContext() or other providers. It would be cool to see that this finally works before moving on.

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

2 participants