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

Added hook for ham menu functionality #19

Merged
merged 5 commits into from
Sep 24, 2020
Merged

Added hook for ham menu functionality #19

merged 5 commits into from
Sep 24, 2020

Conversation

asharonbaltazar
Copy link
Contributor

@asharonbaltazar asharonbaltazar commented Sep 22, 2020

Edit: Fixes #18
The menu is implemented with toggle functionality and works as expected. No need for custom JS — possible with a useState hook

Copy link
Member

@arnavb arnavb left a comment

Choose a reason for hiding this comment

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

Functionality-wise, this seems good. Could you also address the other part of the issue, the CSS transition for the dropdown?

@arnavb arnavb added the bug Something isn't working label Sep 22, 2020
@arnavb arnavb assigned Abh1T and asharonbaltazar and unassigned Abh1T Sep 22, 2020
@asharonbaltazar
Copy link
Contributor Author

Sure. Any particular way you'd like me to achieve that?

@arnavb
Copy link
Member

arnavb commented Sep 23, 2020

I think adding a CSS transition to the active class would be the best approach. This issue has some examples.

@asharonbaltazar
Copy link
Contributor Author

I think CSS is too messy, IMO, especially with a pre-existing library. React has plenty of animation libraries, though.

I'll just stop here. You can either accept my PR or just close it. Sorry about that.

Copy link
Member

@arnavb arnavb left a comment

Choose a reason for hiding this comment

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

As far as CSS transition goes, it really isn't going to bloat anything since it's just simple code to add (I wrote the code below by tweaking the code from that issue a little bit). Try doing something similar and adding it. In contrast to what you said, a React transition library will add much more bloat than just 2 CSS rule blocks.

.navbar-menu {
  display: block;
  opacity: 0;

  position: absolute;
  left: 0;
  right: 0;

  transform: translateY(-100%);
  transition: all 0.2s cubic-bezier(0.3, 0.01, 1, -0.09);
  pointer-events: none;
}

.navbar-menu.is-active {
  opacity: 1;
  transform: none;
  pointer-events: auto;
}

src/App.js Show resolved Hide resolved
Copy link
Member

@arnavb arnavb left a comment

Choose a reason for hiding this comment

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

Even though the transition itself is incomplete, I'm merging this in because someone else will work on getting that set up.

@arnavb arnavb merged commit 1ff0b51 into InVisionAR:master Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hamburger menu does not work on mobile
4 participants