Nav bar by dave5801 · Pull Request #854 · OperationCode/operationcode_frontend

dave5801 · 2018-02-11T04:32:23Z

Description of changes

This issue addresses:

I changed the css so that the logo and navbar remained fixed to the page.

Issue Resolved

Fixes #

jjhampton · 2018-02-12T03:38:58Z

@@ -1,8 +1,10 @@
 .topNav {
  display: flex;
+  margin-left: 400px;


There should be a better way to center this text, other than using hardcoded px numbers and margin-left. Maybe look at using flexbox properties and adjusting the heirarchy of layout elements?

jjhampton

@dave5801 The fixed header has issues when scrolling down - the logo & nav items are still visible over the rest of the page:

Also, changing background-color of the header on non-landing pages (Code Schools, Job Board, Leadership Circle, etc) to black is a pretty drastic change. We should aim to use some of the standardized colors in the color palette if at all possible (see https://operationcode.org/branding). Though the original color of rgb(117,129,143) is not listed there, it matches that color scheme better, IMO.

jjhampton · 2018-02-16T04:26:30Z

@dave5801 I'm still seeing the bug from the earlier screenshot where the logo & nav items are still visible over the rest of the page, when scrolling down on the landing page.

The nav also doesn't seem to be taking up the full width of the screen, visible on the other screens:

Looks like that can be remedied by not using position: fixed.

I've have an idea for a sticky header using CSS position: sticky that could be transparent when it's at the top of the page (to allow full visibility of the landing page image of the family), but changes style when scrolled lower (to a more opaque color). Do you mind if I try and add some commits to your fork so we can get this issue finished up? You may need to enable Allow edits from maintainers.

…de_frontend into nav-bar

kylemh · 2018-02-18T20:21:23Z

@jjhampton just to keep the issues scoped well - let's not worry about the too many objects making the navbar span two rows here. I just want to focus on having the navbar be an element without a nested div. I'm curious on how to work @ksmacleod99 's HeroBanner component in with this setup tho

dave5801 · 2018-02-19T20:51:25Z

@kylemh @jjhampton I have moved the bgimage from the root node into its own div, the nav bar and family image are now independent of each other. However, it opens up a new issue. That is, the bg image is in drastic need of resize relative to mobile/desktop views. A slight resizing of my browser window causes the image to dramatically shrink. I believe, some modification of the home.css will solve this.

jjhampton · 2018-02-25T20:32:36Z

@dave5801 Yeah, looks like there will still need to be some CSS adjustments, the hero image doesn't appear to be correctly positioned at this point. Let's get some clarity on requirements before adding further changes:

@kylemh Can you review at this when you get a chance? It seems like we need some clarity in terms of what issue #459 requires - the issue lacks a description, so all we currently have to work off of are:

'Put Nav at top of page, not on top of landing banner.' (issue title)
'focus on having the navbar be an element without a nested div'

This will ensure that unneeded CSS / layout changes aren't added that are out of the scope of this issue.

dave5801 · 2018-02-26T16:52:25Z

@jjhampton Yes, when I finished the navbar it basically disrupted the sizing the hero image. I wanted to perform the adjustments, however I believe that @ksmacleod99 had addressed this problem in her pull request, but I'm uncertain. I can adjust the CSS if needed.

kylemh · 2018-03-06T07:04:27Z

-  padding: 0 2rem;
+  /*padding: 0 2rem;*/
+  width: 100%;
+  position: -webkit-sticky;


We have an auto-prefixer built into our webpack build script. You won't need prefixes!

kylemh · 2018-03-06T07:04:41Z

  height: 80px;
  background-color: rgb(117,129,143);
-  padding: 0 2rem;
+  /*padding: 0 2rem;*/


Simply delete lines you do not want, don't comment them out.

kylemh · 2018-03-06T07:05:06Z

@@ -1,16 +1,21 @@
 .header {
  display: flex;
+  justify-content:space-around; 


Don't forget a space between the attribute and the value! I also highly recommend setting up style lint.

kylemh · 2018-03-06T07:29:28Z

    mentor: false,
  };

+  /**/


Don't forget to delete this line.

kylemh · 2018-03-06T07:37:32Z

-        }
-      >
+      <div>
+        <div


So, once we get @ksmacleod99 's work in, you'll drop this div and insert the specific <HeroBanner /> for the home route, and then in the relevant files for the routes that use HeroBanners (history, our_programs, team)

jjhampton · 2018-03-06T16:29:39Z

@dave5801 re: commit 082a8a5 - "did an npm install" - we're using Yarn on this project, so please run yarn instead of npm install. There is no reason for you to add "font-awesome": "^4.7.0" to the package.json as part of this issue, please remove that change.

Instead, please merge upstream/master into your feature branch, and then simply run yarn.

re: 2dbcec8 - "I'm assuming HeroBanner exists" - it does not yet, that will be added in PR #871 (currently still under review). So if you want to use the HeroBanner, you will need to wait until that is merged in.

dave5801 · 2018-03-06T16:32:42Z

@kylemh -- I made the requested changes. However, I made an assumption that the HeroeBanner component is where I think it is (I didn't see it after doing a pull origin of master) -- as a result I got a travis build fail.

kylemh · 2018-05-06T04:21:19Z

@dave5801 the HeroBanner component is merged. You should feel free to progress, and make changes to the component as are necessary. Please note these details.

I'm also available for help if you need it! 👍

dmarchante · 2018-07-14T21:36:36Z

@jjhampton @kylemh Can we make the requested changes, fix the conflicts, close the PR, then open another issue for the remaining requirements?

jjhampton · 2018-07-17T23:49:36Z

@dmarchante I vote no on that - I'm unclear on exactly what issue #459 requires to be complete. Specifically, this:

'Put Nav at top of page, not on top of landing banner.' (issue title)
'focus on having the navbar be an element without a nested div'

The issue originally seemed focused on adjusting the navbar, but there's since been some discussion of adding in changes to make the HeroBanner reusable as well.

I'd prefer to close the original issue and simply open new issues (if needed) that have clear acceptance criteria.

dmarchante · 2018-07-18T00:31:18Z

@jjhampton I am good with your proposal. Please move forward how you see fit, I would like to clean up a lot of the stray issues.

jjhampton · 2018-08-15T02:01:40Z

Closing, this has been open for 6 months and will need significant additional changes to address issue #459.

kylemh · 2018-08-15T18:12:54Z

Additional info: We're going to leave the nav unaddressed in this old repository, and prepare any nav improvements on the new repo instead.

dave5801 added 2 commits February 10, 2018 20:11

nav bar is now fixed, need to change background back

863c16e

changed backgound colors to original, opcode logo fixed

d231bc2

jjhampton reviewed Feb 12, 2018

View reviewed changes

jjhampton suggested changes Feb 12, 2018

View reviewed changes

changed header.css, made header position fixed

0a4655c

dave5801 added 5 commits February 16, 2018 14:46

Merge branch 'master' of https://github.com/operationcode/operationco…

23a975b

…de_frontend into nav-bar

changed header width to 100%, commented out padding

649f551

sticky nav works, commented out .header overflow:hidden

073d074

just saving progress

99c22b3

removed transparent property from Header in home.js

161ca0f

moved bgimage out of root node into its own div

dbcabd9

kylemh suggested changes Mar 6, 2018

View reviewed changes

dave5801 added 3 commits March 6, 2018 07:02

did an npm install

082a8a5

Merge branch 'master' into nav-bar

2de0928

made changes as per PR feedback, I'm assuming HeroBanner exists

2dbcec8

removed font awesome from package.json

13e7072

jjhampton closed this Aug 15, 2018

Conversation

dave5801 commented Feb 11, 2018

Description of changes

Issue Resolved

Uh oh!

jjhampton Feb 12, 2018

Choose a reason for hiding this comment

Uh oh!

jjhampton left a comment

Choose a reason for hiding this comment

Uh oh!

jjhampton commented Feb 16, 2018

Uh oh!

kylemh commented Feb 18, 2018

Uh oh!

dave5801 commented Feb 19, 2018

Uh oh!

jjhampton commented Feb 25, 2018

Uh oh!

dave5801 commented Feb 26, 2018

Uh oh!

kylemh Mar 6, 2018

Choose a reason for hiding this comment

Uh oh!

kylemh Mar 6, 2018

Choose a reason for hiding this comment

Uh oh!

kylemh Mar 6, 2018

Choose a reason for hiding this comment

Uh oh!

kylemh Mar 6, 2018

Choose a reason for hiding this comment

Uh oh!

kylemh Mar 6, 2018

Choose a reason for hiding this comment

Uh oh!

jjhampton commented Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dave5801 commented Mar 6, 2018

Uh oh!

kylemh commented May 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmarchante commented Jul 14, 2018

Uh oh!

jjhampton commented Jul 17, 2018

Uh oh!

dmarchante commented Jul 18, 2018

Uh oh!

jjhampton commented Aug 15, 2018

Uh oh!

kylemh commented Aug 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jjhampton commented Mar 6, 2018 •

edited

Loading

kylemh commented May 6, 2018 •

edited

Loading