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

Refactor top navigation #599

Closed
wants to merge 12 commits into from
Closed

Refactor top navigation #599

wants to merge 12 commits into from

Conversation

baohouse
Copy link
Contributor

@baohouse baohouse commented Sep 6, 2017

Addresses #595

  • Make nav no longer push content below it (desktop)
  • Make nav no longer push content below it (mobile)
  • Fix display of secondary navigation when hovering over other primary nav links
  • Enhance some colors
  • Add CSS transition animation (desktop)
  • Add CSS transition animation (mobile)
  • Add missing Home link in mobile size
  • Fix display of two active links in mobile size while on Categories or Moods pages.

Addresses #595

1. Changed behavior in which top nav no longers pushes content below it.
2. Fixed display of secondary navigation when hovering over other primary nav tabs.
3. Updated colors.
@baohouse baohouse added the wip label Sep 6, 2017
@baohouse baohouse self-assigned this Sep 6, 2017
@baohouse baohouse requested review from nma and abrophy September 6, 2017 09:28
@abrophy
Copy link
Collaborator

abrophy commented Sep 6, 2017

Tested locally, though the top-nav behavior has been addressed, some of the JS in that file that was dealing with the moments UI makes me want to hold off on merging it until we have the react version up, will review as soon as that's been taken care of

@baohouse
Copy link
Contributor Author

baohouse commented Sep 6, 2017

@abrophy Heck yeah! React here we come! I'll finish up a few things on this branch so that we have a finalized HTML structure / CSS classnames, and then I'll make another branch to convert these changes into React components.

Moved to using a container for the secondary navigations and control the animation height for the container rather than for each individual secondary navigation list.
@abrophy
Copy link
Collaborator

abrophy commented Sep 8, 2017

Was unnecessarily concerned about possible conflicts with react, have tested it and it looks to be working

@nma
Copy link
Collaborator

nma commented Sep 22, 2017

Mind if I use snippets from this commit in the documentation task on how to use React? :)

Glad to see that the react infrastructure upgrade is seeing use now 👍

@baohouse
Copy link
Contributor Author

@nma Oh, it's far from done, but yeah, it'd be good to document a primer on how to build a React component and embed it into the ERB.

@baohouse
Copy link
Contributor Author

@timofonic #691 Looks like we'll want to focus on that; this PR here is minor and will be obsolete by #691.

@julianguyen julianguyen closed this Feb 4, 2018
@baohouse baohouse deleted the top-navigation-refactor branch March 21, 2018 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants