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

Removed mobile hamburger icon and added button to close mobile menu #120

Merged
merged 2 commits into from Nov 14, 2019

Conversation

abowler2
Copy link
Collaborator

Mobile menu required users to click off the navbar in order to close it.

The suggested fix was to set the navbar to close when a nav link is clicked. However, I ran up against issues with getting the link to apply to only those nav items without children given the way they are mapped.

This fix adds a button to the top right corner allowing the user to close the menu after making their selection. If it is preferred to have the originally suggested behavior of closing upon selection of a nav item I would be happy to work on it some more.

Also, while working on this I noticed that both the mobile hamburger menu icon and the text "Menu" were being rendered. I removed the hamburger icon and left just the "Menu" text.

…verted to hamburger, padding right removed to match position of logo
@lloan
Copy link
Member

lloan commented Nov 14, 2019

@abowler2 I went ahead and modified this a bit, thanks a ton for the work! based on the talk we had yesterday, i went in to the file and modified the nav items that don't have children and made it so that they can close the offcanvas. I also reverted the menu text to the hamburger icon, felt like it balances out the design a bit more than just the text.

Based on this new user experience though, we might want to add a 'to the top' component or since the nav button isn't sticky, we might want to add a menu icon that is hovering on the bottom right. Will most likely open up a new issue for this as a new feature once I figure out the details.

Thanks a ton!

@lloan lloan merged commit 56f9be3 into master Nov 14, 2019
@abowler2
Copy link
Collaborator Author

I like this fix a lot! Much better UX 😄

@lloan lloan deleted the close-mobile-menu branch November 16, 2019 10:15
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