improve some stuff around the nav #188

kentcdodds · 2017-07-01T19:53:45Z

What: Moves the nav to the left.

Why: Closes #121

How: code and stuff

codecov · 2017-07-01T20:01:40Z

Codecov Report

Merging #188 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #188      +/-   ##
==========================================
+ Coverage   96.94%   96.99%   +0.04%     
==========================================
  Files          29       29              
  Lines         262      266       +4     
  Branches       32       32              
==========================================
+ Hits          254      258       +4     
  Misses          8        8

Impacted Files	Coverage Δ
components/hero.js	`100% <ø> (ø)`	⬆️
pages/index/index.js	`100% <ø> (ø)`	⬆️
components/docs-page.js	`100% <ø> (ø)`	⬆️
styles/global-styles.js	`100% <ø> (ø)`	⬆️
components/nav.js	`83.33% <100%> (+5.55%)`	⬆️
components/layout.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca058f9...aff71d6. Read the comment docs.

kentcdodds · 2017-07-01T20:04:38Z

Could still use a little work (especially on mobile) but it's a good start: https://en-rlyghioczn.now.sh/

paulmolluzzo

I may be overlooking something since I'm on mobile, but where is this nav not at the top of the page? I can take a closer look later, but there are some small changes that we should definitely correct just to maintain a consistent look before merging. (Adding the prop to the nav on all pages.)

paulmolluzzo · 2017-07-01T20:33:35Z

components/layout.js

@@ -37,18 +37,25 @@ const Wrapper = glamorous.div((props, {fonts, colors}) => ({
  },
 }))

-function Layout({pathname, children, contributors}) {
+function Layout({pathname, children, contributors, topNav = false}) {


Why do we want to default it to false?

Docs pages (most of the pages) will have it on the left.

Ok, it makes more sense after seeing the desktop version.

paulmolluzzo · 2017-07-01T20:35:21Z

components/nav.js

+    [mediaQueries.mediumUp]: {
+      display: 'flex',
+      justifyContent: top ? 'center' : 'flex-start',
+      flexDirection: top ? 'row' : 'column',


With this line the mobile version is breaking on any page we don't update to have top={true}. Since this is a new prop only index and Getting Started are correct on mobile right now.

If you move the mediumDown media query underneath this I think it'll fix the mobile styles for all pages. The idea is that it's sidebar on desktop wherever we don't say it's a top nav, and across the top on mobile on all pages without needing a prop.

Also need to override the width, maxHeight, justifyContent, and flexDirection on mobile.

paulmolluzzo · 2017-07-01T20:36:10Z

pages/getting-started/index.js

@@ -57,7 +57,7 @@ function CodePreview() {

 function Page({url}) {
  return (
-    <Layout pathname={url ? url.pathname : ''}>
+    <Layout pathname={url ? url.pathname : ''} topNav={true}>


Needs to be on all pages.

Only applies to getting-started and index. All the others are not on the top.

Even on mobile? I would think that on mobile it would be on top on all pages.

This is what I'm referring to:

Sorry, on mobile we'll use media queries I think.

paulmolluzzo · 2017-07-01T20:37:20Z

components/docs-page.js

@@ -25,7 +25,7 @@ const PageWrapper = glamorous.div((props, {colors}) => ({
  '& > h3': {
    width: '100%',
    margin: '20px auto',
-    maxWidth: '50rem',
+    maxWidth: '70rem',


This should be a global variable or something I think... 🤔

wcastand · 2017-07-01T20:45:06Z

Im on iPhone right now so I looked a bit more later but in iPhone I have a different color for the bottom border of the menu to the left
I don't know if that's something you wanted or not.

On mobile the button get started is really big or the text is too long

Other than that, it looks really cool on iPhone :)
Looking forward the desktop version :)

paulmolluzzo · 2017-07-01T20:54:52Z

Im on iPhone right now so I looked a bit more later but in iPhone I have a different color for the bottom border of the menu to the left
I don't know if that's something you wanted or not.

@wcastand the border is a div with three colors left to right. That's how it was designed. 😀

paulmolluzzo · 2017-07-01T21:05:41Z

Three more issues for me:

The default option is missing in the nav I think:

The colors for contributors and the sidebar is identical, so this part looks odd. Maybe we want a right border on the sidebar?

paulmolluzzo · 2017-07-01T22:23:46Z

components/layout.js

-        <Div position="relative" zIndex={1}>
-          <Div display="flex" justifyContent="flex-end" alignItems="center">
-            <Nav pathname={pathname} />
+        <Div position="relative" zIndex={1} display={topNav ? null : 'flex'}>


This needs to be responsive to media queries, so probably want to pull this out of the inline css props syntax.

paulmolluzzo · 2017-07-01T22:23:54Z

components/layout.js

+        <Div position="relative" zIndex={1} display={topNav ? null : 'flex'}>
+          <Div
+            display="flex"
+            justifyContent={topNav ? 'flex-end' : 'flex-start'}


Dupe: This needs to be responsive to media queries, so probably want to pull this out of the inline css props syntax.

paulmolluzzo · 2017-07-01T22:23:59Z

components/layout.js

+            display="flex"
+            justifyContent={topNav ? 'flex-end' : 'flex-start'}
+            alignItems="center"
+            flexDirection={topNav ? 'row' : 'column'}


Dupe: This needs to be responsive to media queries, so probably want to pull this out of the inline css props syntax.

paulmolluzzo · 2017-07-01T22:24:05Z

components/layout.js

+          >
+            <Nav pathname={pathname} top={topNav} />
+          </Div>
+          <Div flex="1">


Dupe: This needs to be responsive to media queries, so probably want to pull this out of the inline css props syntax.

paulmolluzzo · 2017-07-02T04:59:53Z

@kentcdodds I pushed some changes to respect the top/sidebar versions and make both responsive for mobile screens. If you take a look and approve then go for it! If you have improvements to make I'll review what you add in.

paulmolluzzo · 2017-07-02T05:03:46Z

Not sure about the failing CI... 😕

kentcdodds · 2017-07-02T05:16:09Z

My bad. I broke the scripts. Will fix in a moment.

kentcdodds · 2017-07-02T05:26:47Z

This is great! I love the opacity stuff.

There are a few issues you'll probably notice pretty quickly as you resize the window. I don't have time to get that fixed in the next few days, so feel free if you have time :)

https://en-glamorous-jabjandwxo.now.sh

…ous-website into pr/improve-nav-styling

…switch nav to largeDown

paulmolluzzo · 2017-07-02T13:44:32Z

@kentcdodds I updated the nav to switch to the "mobile" layout on tablets. The overflowing nav is too much content to make look good once you get around 1000px.

I also increased the h1 size on mobile and tablets. It was way too small.

kentcdodds · 2017-07-02T14:33:24Z

Looking much better! But we need to fix this (shouldn't clip the right off).

paulmolluzzo · 2017-07-02T14:42:03Z

Sorry, can you circle the issue? IDK what I'm looking for. 😅

kentcdodds · 2017-07-02T14:52:55Z

paulmolluzzo · 2017-07-02T15:15:05Z

It's the flex. Off to the pool now so I'll fix later. 😎

improve some stuff around the nav

49bf625

kentcdodds requested a review from paulmolluzzo July 1, 2017 19:53

kentcdodds mentioned this pull request Jul 1, 2017

Move the nav menu and maybe the Title/logo/button on the homepage #121

Closed

paulmolluzzo suggested changes Jul 1, 2017

View reviewed changes

paulmolluzzo reviewed Jul 1, 2017

View reviewed changes

fix(layout,nav) Make nav responsive for top nav and sidebar

97722bb

paulmolluzzo approved these changes Jul 2, 2017

View reviewed changes

Kent C. Dodds and others added 3 commits July 1, 2017 23:17

improve some stuff around the nav

03ab36f

fix(layout,nav) Make nav responsive for top nav and sidebar

44da679

fix build scripts

7c5933b

kentcdodds force-pushed the pr/improve-nav-styling branch from 97722bb to 7c5933b Compare July 2, 2017 05:18

paulmolluzzo added 3 commits July 2, 2017 08:21

Merge branch 'pr/improve-nav-styling' of github.com:kentcdodds/glamor…

df251f5

…ous-website into pr/improve-nav-styling

refactor(nav, layout, global-styles) Add in largeDown breakpoint and …

98186f1

…switch nav to largeDown

refactor(hero) Increase font size of mobile h1

609efaf

Kent C. Dodds added 2 commits July 2, 2017 10:16

fix overflow

d196484

update width

aff71d6

kentcdodds merged commit 663e539 into master Jul 2, 2017

kentcdodds deleted the pr/improve-nav-styling branch July 2, 2017 16:47

paulmolluzzo mentioned this pull request Jul 2, 2017

Improve mobile #189

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve some stuff around the nav #188

improve some stuff around the nav #188

kentcdodds commented Jul 1, 2017

codecov bot commented Jul 1, 2017 •

edited

kentcdodds commented Jul 1, 2017

paulmolluzzo left a comment

paulmolluzzo Jul 1, 2017

kentcdodds Jul 1, 2017

paulmolluzzo Jul 1, 2017

paulmolluzzo Jul 1, 2017

paulmolluzzo Jul 1, 2017

paulmolluzzo Jul 1, 2017

paulmolluzzo Jul 1, 2017

kentcdodds Jul 1, 2017

paulmolluzzo Jul 1, 2017

paulmolluzzo Jul 1, 2017

kentcdodds Jul 1, 2017

paulmolluzzo Jul 1, 2017

wcastand commented Jul 1, 2017

paulmolluzzo commented Jul 1, 2017

paulmolluzzo commented Jul 1, 2017

paulmolluzzo Jul 1, 2017

paulmolluzzo Jul 1, 2017

paulmolluzzo Jul 1, 2017

paulmolluzzo Jul 1, 2017

paulmolluzzo commented Jul 2, 2017

paulmolluzzo commented Jul 2, 2017

kentcdodds commented Jul 2, 2017 •

edited

kentcdodds commented Jul 2, 2017

paulmolluzzo commented Jul 2, 2017

kentcdodds commented Jul 2, 2017

paulmolluzzo commented Jul 2, 2017

kentcdodds commented Jul 2, 2017

paulmolluzzo commented Jul 2, 2017

improve some stuff around the nav #188

improve some stuff around the nav #188

Conversation

kentcdodds commented Jul 1, 2017

codecov bot commented Jul 1, 2017 • edited

Codecov Report

kentcdodds commented Jul 1, 2017

paulmolluzzo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wcastand commented Jul 1, 2017

paulmolluzzo commented Jul 1, 2017

paulmolluzzo commented Jul 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmolluzzo commented Jul 2, 2017

paulmolluzzo commented Jul 2, 2017

kentcdodds commented Jul 2, 2017 • edited

kentcdodds commented Jul 2, 2017

paulmolluzzo commented Jul 2, 2017

kentcdodds commented Jul 2, 2017

paulmolluzzo commented Jul 2, 2017

kentcdodds commented Jul 2, 2017

paulmolluzzo commented Jul 2, 2017

codecov bot commented Jul 1, 2017 •

edited

kentcdodds commented Jul 2, 2017 •

edited