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

Extract Nav component that has a mobile view #1

Merged
merged 7 commits into from May 12, 2018

Conversation

2 participants
@dagda1
Copy link

dagda1 commented May 7, 2018

Before I go any further, I'd like to know that this is the type of thing you are looking for.

The Sidebar moves to the top at a breakpoint of 600px.

I'll await your feedback before going any futher.

I have some tidying up and styling of the nav in mobile view but lets see if this is what you want first.

@bvaughn bvaughn self-assigned this May 8, 2018

@bvaughn
Copy link
Owner

bvaughn left a comment

This is a nice start!

I have not tested it much, but in a small Chrome window and the iOS simulator, it looks nice.

@@ -5,6 +5,7 @@
"private": true,
"license": "MIT",
"dependencies": {
"font-awesome": "^4.7.0",

This comment has been minimized.

@bvaughn

bvaughn May 8, 2018

Owner

nit: Looks like you forgot to update the Yarn lock file.

That being said, I'd like to avoid pulling in a dependency on font-awesome's stylesheets if avoidable. What is this giving us besides an arrow icon? (Can we just make our own?)

This comment has been minimized.

@bvaughn

bvaughn May 8, 2018

Owner

As a side note, I kind of prefer the hamburger (3 horizontal line) icon style to the spinning arrow anyway 😄 We could probably just snag this from Google's Material SVGs.

import FixedSizeGridExample from './routes/examples/FixedSizeGrid';
import FixedSizeListExample from './routes/examples/FixedSizeList';
import FixedSizeListWithScrollingIndicatorExample from './routes/examples/FixedSizeListWithScrollingIndicator';
import ScrollToItemExample from './routes/examples/ScrollToItem';

This comment has been minimized.

@bvaughn

bvaughn May 8, 2018

Owner

All of these formatting changes probably weren't intentional?

This comment has been minimized.

@dagda1

dagda1 May 8, 2018

no they were not intentional. I'm using vscode and I thought it would have picked up the .prettierrc. Where is your setting for double quote strings?

This comment has been minimized.

@bvaughn

bvaughn May 8, 2018

Owner

Hm. I think it's actually a problem with my Code settings. 😅 My only concern at the moment is that it will complicate merging. So I should get it sorted out in master I guess.

@dagda1

This comment has been minimized.

Copy link

dagda1 commented May 8, 2018

@bvaughn removed font-awesome and used hamburger. Will have another play tonight

@bvaughn

This comment has been minimized.

Copy link
Owner

bvaughn commented May 8, 2018

Sounds great. No hurry.

@dagda1

This comment has been minimized.

Copy link

dagda1 commented May 8, 2018

@bvaughn I would normally use sass so I can be consistent about media queries and use vars for breakpoints, e.g.

@mixin mq-xs {
  @include mq($from: gel-bp-xs) {
    @content;
  }
}

But sass might be off putting for some people. I've used styled components but I am not a fan of css-in-js.

Plain css feels weird but maybe for now it is fine.

@bvaughn

This comment has been minimized.

Copy link
Owner

bvaughn commented May 8, 2018

Not a fan of using SASS or LESS.

I think at this point, I'm not ready to commit to ejecting from create-react-app so CSS modules is probably the best available option. I might look into trying CSS blocks or just using Glamor again if I decided to eject for other reasons.

I'd say let's not worry about this for now. 😄

@dagda1 dagda1 force-pushed the dagda1:nav branch from 98f80d5 to 268a05c May 8, 2018

@dagda1

This comment has been minimized.

Copy link

dagda1 commented May 8, 2018

@bvaughn how about that for a first move to a more responsive layout. I've got a pure css hamburger menu now on the left and I've grabbed the available width for the body below the menu in mobile view.

Is there anything else you thing is needed?

@dagda1

This comment has been minimized.

Copy link

dagda1 commented May 9, 2018

Another idea is to have an overlay rather than a drawer that slides out from the side (which I hate) or my implementation of the top sliding down is to have an overlay menu like this

There is no reason why the sidebar menu should not take over the whole screen in mobile view.

@dagda1

This comment has been minimized.

Copy link

dagda1 commented May 10, 2018

Hi @bvaughn, any feedback?

@bvaughn

This comment has been minimized.

Copy link
Owner

bvaughn commented May 10, 2018

Sorry, @dagda1. This is a side project effort for me. My feedback will be a little slow.

I like the new hamburg icon style better. 😄

There are some changes to yarn.lock and package-lock.json files in this PR now that I would not expect. Maybe revert them?

Another idea is to have an overlay rather than a drawer that slides out from the side (which I hate) or my implementation of the top sliding down is to have an overlay menu like this

I think the behavior you linked too looks reasonable. It's similar to how the reactjs.org mobile nav works!

@dagda1

This comment has been minimized.

Copy link

dagda1 commented May 10, 2018

@bvaughn I was not trying to hurry you up :). I do like that menu. I'll try that and see what it looks like.

Paul Cowan added some commits May 11, 2018

Paul Cowan
Merge remote-tracking branch 'upstream/master' into nav
* upstream/master:
  Slightly tweaked example code
@bvaughn

This comment has been minimized.

Copy link
Owner

bvaughn commented May 12, 2018

Thanks again for this PR 😄 I'm going to merge it and play around with it some.

@bvaughn bvaughn merged commit 6cb2c7e into bvaughn:master May 12, 2018

@dagda1

This comment has been minimized.

Copy link

dagda1 commented May 12, 2018

👍

@bvaughn

This comment has been minimized.

Copy link
Owner

bvaughn commented May 12, 2018

Pushed to react-virtualized-v10.now.sh after iterating a bit! Thanks again!

@dagda1

This comment has been minimized.

Copy link

dagda1 commented May 12, 2018

@bvaughn looks good. I think the lesson learned for me is that a sidebar like this in mobile view should just take up the whole screen. Absolutely no reason not.

@bvaughn

This comment has been minimized.

Copy link
Owner

bvaughn commented May 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment