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

fix: Menu Drop down #1342

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

Conversation

snmln
Copy link
Contributor

@snmln snmln commented Dec 18, 2024

Close the new USWDS page header dropdown when clicking outside of it
#1296

Validation / Testing

Open one of the two drop down options, click on another link on the page. When the browser navigates to the new page that original dropdown menu should be closed.

Copy link

netlify bot commented Dec 18, 2024

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit 5acc803
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/67641dea2c06f000089029c6
😎 Deploy Preview https://deploy-preview-1342--veda-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@snmln snmln linked an issue Dec 18, 2024 that may be closed by this pull request
@snmln snmln changed the title Fix: Menu Drop down fix: Menu Drop down Dec 18, 2024
@snmln snmln marked this pull request as ready for review December 18, 2024 20:45
@snmln snmln force-pushed the 1296-page-header-close-dropdown-when-clicking-outside branch from b1c9d34 to 5acc803 Compare December 19, 2024 13:21
@@ -0,0 +1,15 @@
import { useEffect, useRef } from 'react';
Copy link
Member

@AliceR AliceR Dec 19, 2024

Choose a reason for hiding this comment

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

Not very important, but maybe we can move both use-fix-hooks in the same directory? app/scripts/components/common/page-header/use-mobile-menu-fix.ts is the one i implemented for the resize bug. I don't care which directory, but I like them together 🙂

Copy link
Collaborator

Choose a reason for hiding this comment

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

@AliceR I think the new hook is generic enough to be useful in other places not limited to this component, so it might make sense to keep it in the general utils directory?

Copy link
Member

@AliceR AliceR left a comment

Choose a reason for hiding this comment

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

This is looking good!
Do we want to add closing the mobile menu to this work, or should this be a separate issue to tackle? It's pretty much the same logic, maybe we just need to setExpanded to false?

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.

Close the new USWDS page header dropdown when clicking outside of it
3 participants