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

Navbarchanges #8

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

Navbarchanges #8

wants to merge 2 commits into from

Conversation

MatthewBorda
Copy link
Collaborator

Added navbar dropdown for Find a Poem

Added dropdown
Added poetry dropdown framework
@MatthewBorda
Copy link
Collaborator Author

Here is a rough outline of the embedded dropdowns. I will work to clean them up

Copy link
Collaborator

@branchaw branchaw left a comment

Choose a reason for hiding this comment

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

Great to have so many search options in the drop downs!

@MikeGalli
Copy link
Collaborator

I admit it is hard to do a layout for the nav bar before the wireframe has been approved. When I looked at the view generated by the code, I saw some things that could be improved. I think:
The nav bar is too tall.
The sign in is too close to the right side.
The line under Home ends randomly.
04_codereview
05_codereview

@lukegreen
Copy link
Collaborator

lukegreen commented Nov 13, 2016

Maybe tweek the CSS on this so that the content in the jumbotron clears the navbar. I think if you put the navbar elements in a div with the following attributes, then it should at least put them inline with the page title element and shave a row off the top of the header: <div class="collapse navbar-collapse" id="js-navbar-collapse">

@lukegreen
Copy link
Collaborator

lukegreen commented Nov 13, 2016

Here's a more detailed template to clarify my previous comment:

<div class="header">
      <div class="navbar navbar-default" role="navigation">
        <div class="container">
          <div class="navbar-header">

            <button type="button" class="navbar-toggle collapsed" data-toggle="collapse" data-target="#js-navbar-collapse">
              <span class="sr-only">Toggle navigation</span>
              <span class="icon-bar"></span>
              <span class="icon-bar"></span>
              <span class="icon-bar"></span>
            </button>

            <a class="navbar-brand" href="#/">RoverTrak: Development</a>
          </div><div class="collapse navbar-collapse" id="js-navbar-collapse">

            <ul class="nav navbar-nav">
              <li class="active"><a href="#/">Home</a></li>
              <li><a href="">About</a></li>
              <li><a href="">Find a Poem</a></li>
              <li><a href="">Find a Drink</a></li>
            </ul>
          </div>
        </div>
      </div>
    </div>

This should also give you a hamburger menu on small (mobile) screen sizes.

@branchaw
Copy link
Collaborator

branchaw commented Nov 16, 2016

I think the overlap issues might be avoided by locating the drop downs inline with the nav bar and below header image as shown in the mockup suggested in Pull Request #12

for eg:
screenshot-ancient-fog-branchaw353819 codeanyapp com 2016-11-15 17-35-59

Copy link
Collaborator

@branchaw branchaw left a comment

Choose a reason for hiding this comment

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

Suggest moving tabs to be inline with main nav bar rather than above. Also incorporate header image put forward in pull request #12

<li role="presentation" class="active"><a href="#">Home</a></li>
<li role="presentation"><a href="#">About</a></li>
<li class="dropdown">
<a href="#" class="dropdown-toggle" data-toggle="dropdown" role="button" aria-haspopup="true" aria-expanded="false">Find a Poem <span class="caret"></span></a>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should be all replaced with something less hardcoded. I would recommend a plugin such as this one: http://mmenu.frebsite.nl/. We had discussed making this menus and submenus contain dynamic content that is based off of either counts from the API we used or popularity of the site users. This plugin contains the ability to dynamically add menu options and has a mobile first format to it. Overall, I think this code is too clunky to be maintained. This should be used only as a proof of concept.

@MatthewBorda
Copy link
Collaborator Author

I'd recommend scrapping this request. Merge the other page pull request and redo the navbar with a plugin only after an API and backend data structure are finalized. Then we could create the dynamic navbar that fits with the structure of the page.

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.

4 participants