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

functionality for fixed navbar #980

Merged
merged 6 commits into from
Aug 11, 2024
Merged

functionality for fixed navbar #980

merged 6 commits into from
Aug 11, 2024

Conversation

atchox
Copy link
Contributor

@atchox atchox commented Jul 25, 2024

This provides functionality for a fixed navbar, which can be enabled by using the stickyNav key in the siteMetadata.js file. The CSS implementation of locking the scroll (by setting the overflow property on the body element) when the mobile nav is open has been replaced with the use of the npm package body-scroll-lock. This allows for more consistent behavior across devices and prevents the page from scrolling to the top when the nav is closed.

screen.mov

Copy link

vercel bot commented Jul 25, 2024

@atchox is attempting to deploy a commit to the timlrx's projects Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Jul 30, 2024

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

Name Status Preview Comments Updated (UTC)
tailwind-nextjs-starter-blog ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 11, 2024 8:30am

@timlrx
Copy link
Owner

timlrx commented Jul 30, 2024

Thanks for the feature contribution, code looks fine to me but appreciate it if you could run it through prettier and push back the changes. I am getting the following errors on vercel:

10:21  Error: Replace `"flex·items-center·bg-white·dark:bg-gray-950·justify-between·py-10"` with `'flex·items-center·bg-white·dark:bg-gray-950·justify-between·py-10'`  prettier/prettier
11:30  Error: Insert `·`  prettier/prettier
12:20  Error: Replace `"·sticky·top-0"` with `'·sticky·top-0'`  prettier/prettier

@atchox
Copy link
Contributor Author

atchox commented Jul 30, 2024

@timlrx ran through prettier. Thanks for the review!

@timlrx
Copy link
Owner

timlrx commented Jul 30, 2024

Hmm I get the following error in the console:

23-ee23754aea40848f.js:1 disableBodyScroll unsuccessful - targetElement must be provided when calling disableBodyScroll on IOS devices.

@atchox
Copy link
Contributor Author

atchox commented Jul 30, 2024

Hmm I get the following error in the console:

23-ee23754aea40848f.js:1 disableBodyScroll unsuccessful - targetElement must be provided when calling disableBodyScroll on IOS devices.

line 184 of bodyScrollLock.js is getting triggered. That's because the div for the mobile nav gets removed when navShow is set to false. So the ref gets set to null and the error is triggered. Would be solved by persistence of the div.

@atchox
Copy link
Contributor Author

atchox commented Jul 30, 2024

  • the issue with the targetElement has been fixed. Some of the nav components are not unmounted when navShow is false.
  • overflow functionality for the nav has been added. See the recordings as example (with placeholder links)
  • padding has been adding to the mobile nav buttons for bigger tap area
desktop.recording.mov
mobile.recording.mov

@timlrx
Copy link
Owner

timlrx commented Aug 11, 2024

I think we are almost there. There's a z-index issue with the code block and video element:
Screenshot 2024-08-11 at 12 42 46 PM

You can replicate it on the /blog/release-of-tailwind-nextjs-starter-blog-v2.0 path.

Otherwise, let's make stickyNav true by default and we can merge it in, thanks!

@atchox
Copy link
Contributor Author

atchox commented Aug 11, 2024

I couldn't recreate the issue with the video component but I fixed it for the code block. Hope this works!

@timlrx
Copy link
Owner

timlrx commented Aug 11, 2024

This works, thanks for the contribution!

@timlrx timlrx merged commit 0320555 into timlrx:main Aug 11, 2024
1 check passed
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.

2 participants