Responsive UI for main menu and banner

Review Request #6796 — Created Jan. 17, 2015 and submitted

jtsengyc
Review Board
master
20d6cc7...
reviewboard, students

Previously without responsive css/js design, the main menu and banner get cramped up when the screen size is smaller than 720px, which is the case for many mobile screens. This reduces the usability of the website in particularly being hard to browse, navigate, and click on the links.

This change focuses on redesigning the main menu and banner to make it easier to use when the screen estate is limited. Including making main menu slide out and search box expandable, and reduces the amount of information shown on the banner.

//
Changes added
- Adjust element displays on width < 720px (+media tags in base.less)
- Logo icon turn into mobile menu toggler when width <= 720px (+control logic in common.js)
- Translucent masking layer sit between mobile menu and rest of the page, closes the menu when clicked (+div dom in base.html)
- Moved account & support menu into its own template file and included in mobile menu (+navsupportmenu.html)
- Using search icon to display/hide search box in mobile menu to reduce screen estate usage
- moved menu toggle logic into it's own backbone view

  • Review board manage.py test
  • Firefox & Chrome window resizing to examine the css change
  • Menu toggle function working when window size <= 720px, including boundary case 720/721px
  • Closing menu by clicking the masking layer
  • Resizing window to > 720px after mobile menu opened will leave the translucent mask on with menu hidden, but the state will remain the same when the window is resized back to <=720px or mask clicked
Loading file attachments...

Description From Last Updated

How about scaling down this icon so that the entire header is the height of the search icon?

daviddavid

Can you stretch the menu vertically to fill the entire height of the document?

daviddavid

Can we remove the dimming when the window gets larger again?

daviddavid

Can we reduce the size of this so that everything is the height of the text / search icon?

daviddavid

This would be better in defs.less. However, we probably want to be more specific in the name, because there's a ...

chipx86chipx86

Space after the :. Given that we're doing this media query a lot, we should have a macro for it ...

chipx86chipx86

Trailing whitespace. Should use .opacity(0.5);

chipx86chipx86

Rather than hard-coding indexes, we should use the pre-defined constants in defs.less.

chipx86chipx86

Space after :.

chipx86chipx86

Should just use 0 instead of 0px. No need for px when dealing with 0.

chipx86chipx86

Space between these.

chipx86chipx86

Any time you're dealing with precise values like these (-160px or the color), you should use constants. Also, don't use ...

chipx86chipx86

Should be more like #mobile-account-nav.

chipx86chipx86

Space after :. Go through the rest of these and make sure they're in the right form.

chipx86chipx86

Trailing whitespace.

chipx86chipx86

Check other JavaScript files for the format needed for doc comments.

chipx86chipx86

Space after if.

chipx86chipx86

We should call this base/_nav_support_menu.html

chipx86chipx86

We only use 1-space indentation for HTML. In this case, don't indent the template tag at all.

chipx86chipx86

Only one blank line here.

chipx86chipx86

Only 1-space indentation.

chipx86chipx86

Can we call this search-icon?

daviddavid

Spaces around the > chacacters.

daviddavid

Space between ) and {

daviddavid

Please break this up onto several lines, and prefer single quotes over double. $('#navbar-container').animate({ left: 0 }, function() { $(this).addClass('menu-active'); ...

daviddavid

These should all be on one line.

daviddavid

You've got some trailing whitespace here. Please also add in punctuation and rewrap.

daviddavid

Space between ) and {

daviddavid

{ on the same line

daviddavid

{ should be on the same line as the if, and } else { should all be one line. You ...

daviddavid

I don't want to overload the term "banner." We already have banners for drafts of reviews and review requests. I'd ...

chipx86chipx86

These are indented via tabs, as are some of the other lines (31, 39, 43), should be spaces. :/

SU Sunxperous

Same tabs and spaces identation issue for this part.

SU Sunxperous

Indentation in LESS files should be 2 spaces.

daviddavid

Please add this line back.

daviddavid

Can you fix these comments?

daviddavid

Wrap to 80 columns (expand out the function to be one line per statement). You've also got some bad indentation ...

daviddavid

It's really weird to use _.template() this way.

daviddavid

No spaces inside ().

daviddavid

Indentation is wacky. Please make sure there are no tab characters and everything is done with spaces.

daviddavid

Indentation.

daviddavid

Indentation.

daviddavid

Space after :. Should also be '0px' as a string rather than just an integer.

daviddavid

Missing semicolon.

daviddavid

Space after :. Should also be '-160px' as a string rather than just an integer.

daviddavid

Missing semicolon.

daviddavid

No need to assign this to a variable, just do new RB.HeaderView(...);

daviddavid

Please undo these changes.

daviddavid

I think you could remove the second line of this comment. It doesn't really add anything.

daviddavid

Because you pass the element in when creating this view, you don't need to specify it here.

daviddavid

Add a space after 'if'

daviddavid

Wrap to 80 columns.

daviddavid

Single quotes.

daviddavid

Single quotes.

daviddavid

Single quotes.

daviddavid

There should be no changes to common.js in this review request.

daviddavid

Remove this blank line.

daviddavid

Please add spaces around >.

daviddavid

Your changes here seem to have regressed the visual appearance for desktop-size windows. Please go through and make sure that ...

daviddavid
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/templates/base/navbar.html
        reviewboard/templates/base/navsupportmenu.html
        reviewboard/templates/base.html
        reviewboard/templates/base/headerbar.html
        reviewboard/static/rb/css/pages/base.less
        reviewboard/static/rb/js/common.js
        reviewboard/templates/base/branding.html
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/templates/base/navbar.html
        reviewboard/templates/base/navsupportmenu.html
        reviewboard/templates/base.html
        reviewboard/templates/base/headerbar.html
        reviewboard/static/rb/css/pages/base.less
        reviewboard/static/rb/js/common.js
        reviewboard/templates/base/branding.html
    
    
  2. 
      
JT
david
  1. When making UI changes, please add lots of screenshots as file attachments.

  2. 
      
JT
JT
chipx86
  1. Hey Jason,

    See my comments on /r/6782/ for what I'm looking to see in summaries/descriptions. This is the first impression to any reviwer, and is just as important as code, which is why you'll see me drill it in :)

  2. 
      
david
  1. I haven't looked at the code yet, just the screenshots. Ditto on what Christian said about the summary/description.

  2. How about scaling down this icon so that the entire header is the height of the search icon?

    1. that would make the icon around 25 x 25px, Might make it too small for clicking on mobile?

    2. If the search icon isn't too small, this icon shouldn't be too small.

  3. Can you stretch the menu vertically to fill the entire height of the document?

  4. Can we remove the dimming when the window gets larger again?

    1. this can be done with adding event listener to window resize, but would that be preferable given this is more of a rare case, the number of event being fired when window is resizing, and the low impact of remaining dimmed?

    2. We can throttle the event handler so it only runs some limited number of times per second (to keep resizing fast). This is a rare case, and you don't have to solve it right away, but I think it should definitely be solved before this change is done.

  5. 
      
JT
chipx86
  1. I left a lot of comments on style, but there's also some higher-level things to go into.

    It's good to familiarize yourself with LessCSS and with how we make use of definitions and macros. You'll need to become an expert in this for this work. We'll probably want to find ways to better organize the new work here to make it more understandable as well. Adding comments to describe the changes making in each media block would be very beneficial.

    Also, we're moving away from common.js. All new JavaScript should be in the model/view setup we have.

  2. This would be better in defs.less. However, we probably want to be more specific in the name, because there's a lot of mobile screen sizes.

  3. Space after the :.

    Given that we're doing this media query a lot, we should have a macro for it in defs.less.

    We actually use a similar thing for the Review Board site, so I'd suggest:

    .on-mobile-medium-screen(@rules) {
      @media screen and (max-width: @medium-screen-width),
             screen and (max-device-width: @medium-screen-width) and (orientation: landscape) {
        @rules();
      }
    }
    

    (Not sure if "medium" is right, but whatever.)

    This will help us have rules for different screen sizes.

    1. would having 720 directly in the variable name be more clarifying?
      eg: @mobile-medium-size-720: 720px

  4. Trailing whitespace.

    Should use .opacity(0.5);

  5. Rather than hard-coding indexes, we should use the pre-defined constants in defs.less.

  6. Space after :.

  7. Should just use 0 instead of 0px. No need for px when dealing with 0.

  8. reviewboard/static/rb/css/pages/base.less (Diff revision 1)
     
     
     

    Space between these.

  9. reviewboard/static/rb/css/pages/base.less (Diff revision 1)
     
     
     

    Any time you're dealing with precise values like these (-160px or the color), you should use constants.

    Also, don't use the long form for the background. You can just specify the color and the rest will go to defaults.

  10. Should be more like #mobile-account-nav.

  11. Space after :.

    Go through the rest of these and make sure they're in the right form.

  12. Trailing whitespace.

  13. reviewboard/static/rb/js/common.js (Diff revision 1)
     
     
     
     
     

    Check other JavaScript files for the format needed for doc comments.

  14. reviewboard/static/rb/js/common.js (Diff revision 1)
     
     

    Space after if.

  15. We should call this base/_nav_support_menu.html

  16. reviewboard/templates/base/navbar.html (Diff revision 1)
     
     
     
     

    We only use 1-space indentation for HTML. In this case, don't indent the template tag at all.

  17. reviewboard/templates/base/navsupportmenu.html (Diff revision 1)
     
     
     
     
     
     

    Only one blank line here.

  18. reviewboard/templates/base/navsupportmenu.html (Diff revision 1)
     
     
     
     

    Only 1-space indentation.

  19. 
      
JT
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/templates/base/navbar.html
        reviewboard/templates/base.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/templates/base/headerbar.html
        reviewboard/static/rb/css/pages/base.less
        reviewboard/static/rb/js/common.js
        reviewboard/templates/base/branding.html
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/templates/base/navbar.html
        reviewboard/templates/base.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/templates/base/headerbar.html
        reviewboard/static/rb/css/pages/base.less
        reviewboard/static/rb/js/common.js
        reviewboard/templates/base/branding.html
    
    
  2. 
      
JT
david
  1. 
      
  2. Can we call this search-icon?

  3. Spaces around the > chacacters.

  4. reviewboard/static/rb/js/common.js (Diff revision 2)
     
     

    Space between ) and {

  5. reviewboard/static/rb/js/common.js (Diff revision 2)
     
     

    Please break this up onto several lines, and prefer single quotes over double.

    $('#navbar-container').animate({
        left: 0
    }, function() {
        $(this).addClass('menu-active');
    });
    

    Same comment for code below.

  6. reviewboard/static/rb/js/common.js (Diff revision 2)
     
     
     
     

    These should all be on one line.

  7. reviewboard/static/rb/js/common.js (Diff revision 2)
     
     
     
     
     
     

    You've got some trailing whitespace here. Please also add in punctuation and rewrap.

  8. reviewboard/static/rb/js/common.js (Diff revision 2)
     
     

    Space between ) and {

  9. reviewboard/static/rb/js/common.js (Diff revision 2)
     
     
     

    { on the same line

  10. reviewboard/static/rb/js/common.js (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    { should be on the same line as the if, and } else { should all be one line.

    You can also do the mobileMenuToggle as one call:

    $.fn.mobileMenuToggle(
        $('#navbar-container').hasClass('menu-active')))
    

    All of this should really go into a backbone view for the header rather than adding stuff to common.js

  11. 
      
david
  1. Jason, can we prioritize this change? All the others depend on it, and I think it would make sense to get this one finished up before tackling too many other things.

  2. 
      
JT
JT
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/templates/base/navbar.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/templates/base/headerbar.html
        reviewboard/static/rb/css/pages/base.less
        reviewboard/static/rb/js/views/bannerView.js
        reviewboard/static/rb/js/common.js
        reviewboard/templates/base/branding.html
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/templates/base/navbar.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/templates/base/headerbar.html
        reviewboard/static/rb/css/pages/base.less
        reviewboard/static/rb/js/views/bannerView.js
        reviewboard/static/rb/js/common.js
        reviewboard/templates/base/branding.html
    
    
  2. 
      
SU
  1. 
      
    1. good eyes for checking, tho the bannerView.js is consistent with other view files using tab instead of space for indentation
      and on the html file, those are only there temporarily (notice it's the same content as bannerView), will be removed once proper insertion of code is put in place.

  2. reviewboard/static/rb/js/views/bannerView.js (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     

    These are indented via tabs, as are some of the other lines (31, 39, 43), should be spaces. :/

  3. reviewboard/templates/base/headerbar.html (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     

    Same tabs and spaces identation issue for this part.

  4. 
      
chipx86
  1. 
      
  2. reviewboard/static/rb/js/views/bannerView.js (Diff revision 3)
     
     
     
     

    I don't want to overload the term "banner." We already have banners for drafts of reviews and review requests. I'd rather we call this something different. It's pretty confusing now.

    1. Probably should be "header".

  3. 
      
JT
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/templates/base/navbar.html
        reviewboard/templates/base.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/templates/base/headerbar.html
        reviewboard/static/rb/css/pages/base.less
        reviewboard/static/rb/js/views/headerView.js
        reviewboard/static/rb/js/common.js
        reviewboard/templates/base/branding.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/templates/base/navbar.html
        reviewboard/templates/base.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/templates/base/headerbar.html
        reviewboard/static/rb/css/pages/base.less
        reviewboard/static/rb/js/views/headerView.js
        reviewboard/static/rb/js/common.js
        reviewboard/templates/base/branding.html
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/static/rb/css/pages/base.less (Diff revision 4)
     
     
     

    Indentation in LESS files should be 2 spaces.

  3. reviewboard/static/rb/js/common.js (Diff revision 4)
     
     

    Please add this line back.

  4. reviewboard/static/rb/js/views/headerView.js (Diff revision 4)
     
     
     
     

    Can you fix these comments?

  5. Wrap to 80 columns (expand out the function to be one line per statement). You've also got some bad indentation here.

  6. It's really weird to use _.template() this way.

  7. No spaces inside ().

  8. reviewboard/static/rb/js/views/headerView.js (Diff revision 4)
     
     
     
     
     
     

    Indentation is wacky. Please make sure there are no tab characters and everything is done with spaces.

  9. Indentation.

  10. Indentation.

  11. Space after :. Should also be '0px' as a string rather than just an integer.

  12. Missing semicolon.

  13. Space after :. Should also be '-160px' as a string rather than just an integer.

  14. Missing semicolon.

  15. reviewboard/templates/base.html (Diff revision 4)
     
     

    No need to assign this to a variable, just do new RB.HeaderView(...);

  16. 
      
JT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/templates/base/navbar.html
        reviewboard/templates/base.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/templates/base/headerbar.html
        reviewboard/static/rb/css/pages/base.less
        reviewboard/static/rb/js/views/headerView.js
        reviewboard/static/rb/js/common.js
        reviewboard/templates/base/branding.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/templates/base/navbar.html
        reviewboard/templates/base.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/templates/base/headerbar.html
        reviewboard/static/rb/css/pages/base.less
        reviewboard/static/rb/js/views/headerView.js
        reviewboard/static/rb/js/common.js
        reviewboard/templates/base/branding.html
    
    
  2. 
      
david
  1. 
      
  2. Can we reduce the size of this so that everything is the height of the text / search icon?

    1. whats the platform/device/setting showing this?
      An updated image of what it looks like will be posted

    2. I'm not quite sure what you mean by this question, but the "Current logo in mobile view" screenshot looks better.

  3. reviewboard/static/rb/js/common.js (Diff revision 5)
     
     
     
     

    Please undo these changes.

    1. this is already an undo of a previous comment. can we have a clear number of line spacing required here?
      currently it's one line space before and after $(document).ready

    2. There's no reason to have a blank line at the beginning of the inner function. Whether it's one or two blank lines before, this is now the only change to common.js, and if we're going to do any code clean-up in it, we should do so as a separate change and not bundled with other functional changes.

  4. I think you could remove the second line of this comment. It doesn't really add anything.

  5. Because you pass the element in when creating this view, you don't need to specify it here.

  6. Add a space after 'if'

  7. Wrap to 80 columns.

  8. Single quotes.

  9. Single quotes.

  10. reviewboard/templates/base.html (Diff revision 5)
     
     

    Single quotes.

  11. 
      
JT
JT
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/templates/base/navbar.html
        reviewboard/templates/base.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/templates/base/headerbar.html
        reviewboard/static/rb/css/pages/base.less
        reviewboard/static/rb/js/views/headerView.js
        reviewboard/static/rb/js/common.js
        reviewboard/templates/base/branding.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/templates/base/navbar.html
        reviewboard/templates/base.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/templates/base/headerbar.html
        reviewboard/static/rb/css/pages/base.less
        reviewboard/static/rb/js/views/headerView.js
        reviewboard/static/rb/js/common.js
        reviewboard/templates/base/branding.html
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/static/rb/js/common.js (Diff revision 6)
     
     

    There should be no changes to common.js in this review request.

  3. Remove this blank line.

  4. Please add spaces around >.

  5. 
      
JT
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/base/navbar.html
        reviewboard/templates/base.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/templates/base/headerbar.html
        reviewboard/static/rb/css/pages/base.less
        reviewboard/static/rb/js/views/headerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/templates/base/branding.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/base/navbar.html
        reviewboard/templates/base.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/templates/base/headerbar.html
        reviewboard/static/rb/css/pages/base.less
        reviewboard/static/rb/js/views/headerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/templates/base/branding.html
    
    
  2. 
      
david
  1. I just applied the change and did a little testing. Here are some suggestions:

    • The overlay mask seems to be left-indented about 0.5em. Please add left: 0 to its style rules.
    • The overlay mask overlays on top of the header bar, shading out the menu button. Can you change the mobile #headerbar padding to be a pixel value (rather that 0.5em) and then change #mobile-menu-mask's top to be aligned such that it's below the header?
    • There doesn't seem to be enough space between the header and the page in mobile mode
    • There's an extra <br> element in the #headerbar.
    • There's not quite enough padding at the top of the mobile menu.
    1. Reviewed

      1. ok
      2. ok
      3. ok - added 10px margin
      4. the <br> element was there originally, is that something we want to remove?
      5. ok - added 10px margin too
    2. For 4, I think yes, we would want to remove it.

  2. 
      
JT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/base/navbar.html
        reviewboard/templates/base.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/templates/base/headerbar.html
        reviewboard/static/rb/css/pages/base.less
        reviewboard/static/rb/js/views/headerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/templates/base/branding.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/base/navbar.html
        reviewboard/templates/base.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/templates/base/headerbar.html
        reviewboard/static/rb/css/pages/base.less
        reviewboard/static/rb/js/views/headerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/templates/base/branding.html
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/static/rb/css/pages/base.less (Diff revisions 7 - 8)
     
     

    Your changes here seem to have regressed the visual appearance for desktop-size windows. Please go through and make sure that things line up and look good in both cases.

    1. Oops...that's one stupid mistake by me. fixed now, let me know if I can have the other pages up for review as well. :)

    2. I'm not sure I understand what you mean.

      Is there a new revision that I can review?

    3. There are a few more currently listed under [WIP] linked below, should I remove the [WIP]?

      6922, #6893, #6923, #6952, #6954

  3. 
      
JT
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/base/navbar.html
        reviewboard/templates/base.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/templates/base/headerbar.html
        reviewboard/static/rb/css/pages/base.less
        reviewboard/static/rb/js/views/headerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/templates/base/branding.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/base/navbar.html
        reviewboard/templates/base.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/templates/base/headerbar.html
        reviewboard/static/rb/css/pages/base.less
        reviewboard/static/rb/js/views/headerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/templates/base/branding.html
    
    
  2. 
      
david
  1. I'm going to make some simple changes to fix up some small issues, and then push this. Thanks!

  2. 
      
JT
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (618f8fc)
Loading...