Skip to content

Close menu and prevent overscroll#4739

Merged
zomars merged 16 commits intomainfrom
close-settings-on-selection
Oct 13, 2022
Merged

Close menu and prevent overscroll#4739
zomars merged 16 commits intomainfrom
close-settings-on-selection

Conversation

@joeauyeung
Copy link
Copy Markdown
Contributor

What does this PR do?

This PR closes the settings side container when the route changes and prevents scrolling past the overlay.

Fixes # (issue)

Environment: Staging(main branch) / Production

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How should this be tested?

  • Navigate to settings on a small screen size
  • Open the container and choose an option
  • With the container open try to scroll past the overlay

Checklist

  • I haven't added tests that prove my fix is effective or that my feature works
  • I haven't checked if new and existing unit tests pass locally with my changes

@vercel
Copy link
Copy Markdown

vercel Bot commented Sep 28, 2022

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

Name Status Preview Updated
cal ✅ Ready (Inspect) Visit Preview Oct 13, 2022 at 1:24PM (UTC)

Comment on lines +316 to +320
useEffect(() => {
if (sideContainerOpen) {
setSideContainerOpen(!sideContainerOpen);
}
}, [router.asPath]);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

missing dependency?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I understand the linting suggestion but I only want this to run when the router path changes. When both dependencies are added, the menu always closes because it triggers when sideContainerOpen changes but if it's open it'll just close automatically.

https://www.loom.com/share/7d453d125c6e4d758e9dcf0447db4345

Copy link
Copy Markdown
Contributor

@zomars zomars left a comment

Choose a reason for hiding this comment

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

LGTM

@zomars zomars merged commit 3af3186 into main Oct 13, 2022
@zomars zomars deleted the close-settings-on-selection branch October 13, 2022 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants