search

parlaywithme / pweb

http://parlaywith.me
0 stars 0 forks source link

Adaptive bottom navbar; reorganized CSS #1

Closed jimjshields closed 10 years ago

jimjshields commented 10 years ago

Made the bottom navbar on the main page adaptive so it changes into a stacked vertical list when under a certain screen size. Cleaned up the CSS organization a bit.

lorensr commented 10 years ago

Hi Jim! The small-screen bottom menu looks nice. I prefer the way flex works on larger screens, gradually increasing spacing. For instance I like the size/spacing of the menu here:

screen shot 2014-11-14 at 1 52 18 pm

over the PR:

screen shot 2014-11-14 at 1 54 43 pm

Could you add that back in, at least for >500px? It might also be a simpler method of making your stacked menu – see the flex-wrap property:

http://css-tricks.com/snippets/css/a-guide-to-flexbox/

jimjshields commented 10 years ago

Totally makes sense - should've realized I took that out too. Will add it back in and take a look at the flex-wrap property for the stacked menu. Thanks!

lorensr commented 10 years ago

Sure, thank you!

jimjshields commented 10 years ago

No problem! I did add that back for the top nav bar (and used the flex to make it a bit more responsive for the moment) but am having an issue with the bottom navbar - I have to run out but should be able to send the changes over tonight/tomorrow morning.

On Fri Nov 14 2014 at 5:15:40 PM Loren Sands-Ramshaw < notifications@github.com> wrote:

Sure, thank you!

— Reply to this email directly or view it on GitHub https://github.com/parlaywithme/pweb/pull/1#issuecomment-63137850.

jimjshields commented 10 years ago

Ok - I just committed it, I've reverted the top navbar to use flex (and added a line to wrap it so it looks marginally better on small screens). The bottom navbar now uses flex and behaves basically the same way (under 500px it turns into green buttons). Let me know if there's anything I missed. Thanks!

jimjshields commented 10 years ago

Oh, and here's what it looks like (large screen, main and product pages, then same for small screen):

large_screen_main large_screen_product small_screen_main small_screen_product

lorensr commented 10 years ago

I like the stacking and the whole width being clickable, but aesthetically prefer green text over green bg. Think green bg might work well for scrolled-down header, since it looks weird to have white cutting off page when you scroll down (eg brown header http://payo.us). In general, I try to use as little markup as possible, so I simplified. And if there's any CSS that's not doing anything anymore, make sure to remove it - not doing so is one of most common ways of building technical debt. When reorging a file, do so in separate commit from other changes so that the other changes show up in the diff.

See https://github.com/parlaywithme/pweb/commit/4c34aab925e024a8002028784c4d8247d1c54fbd

lorensr commented 10 years ago

Also, convention for class + id naming is dashes vs underscores, so "bottom-nav"

jimjshields commented 10 years ago

Makes sense on all points - you're right on the green text, looks better now. I'll keep any changes I submit as discrete as possible, especially if reorganizing anything. And I'll brush up on the CSS naming conventions. Thanks for taking a look and making it better - haven't done a lot of collaborative work so it's really helpful for me.

On Sun Nov 16 2014 at 7:21:57 PM Loren Sands-Ramshaw < notifications@github.com> wrote:

Also, convention for class + id naming is dashes vs underscores, so "bottom-nav"

— Reply to this email directly or view it on GitHub https://github.com/parlaywithme/pweb/pull/1#issuecomment-63247186.

lorensr commented 10 years ago

Welcome! Actually, neither have I, so feel free to give me feedback :) First time merging a pull request even.

On Sun, Nov 16, 2014 at 5:24 PM, jimjshields notifications@github.com wrote:

Makes sense on all points - you're right on the green text, looks better now. I'll keep any changes I submit as discrete as possible, especially if reorganizing anything. And I'll brush up on the CSS naming conventions. Thanks for taking a look and making it better - haven't done a lot of collaborative work so it's really helpful for me.

On Sun Nov 16 2014 at 7:21:57 PM Loren Sands-Ramshaw < notifications@github.com> wrote:

Also, convention for class + id naming is dashes vs underscores, so "bottom-nav"

— Reply to this email directly or view it on GitHub https://github.com/parlaywithme/pweb/pull/1#issuecomment-63247186.

— Reply to this email directly or view it on GitHub https://github.com/parlaywithme/pweb/pull/1#issuecomment-63249900.

jimjshields commented 10 years ago

Hah then I feel a little better - all good so far, definitely helpful feedback. Don't worry, I'll let you know if it's not.