Responsive UI for main menu and banner
Review Request #6796 — Created Jan. 17, 2015 and submitted
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
| Description | From | Last Updated |
|---|---|---|
|
Can you stretch the menu vertically to fill the entire height of the document? |
 david
|
|
|
Can we remove the dimming when the window gets larger again? |
david
|
|
|
How about scaling down this icon so that the entire header is the height of the search icon? |
david
|
|
|
Can we reduce the size of this so that everything is the height of the text / search icon? |
david
|
|
|
This would be better in defs.less. However, we probably want to be more specific in the name, because there's a … |
 chipx86
|
|
|
Space after the :. Given that we're doing this media query a lot, we should have a macro for it … |
chipx86
|
|
|
Trailing whitespace. Should use .opacity(0.5); |
chipx86
|
|
|
Rather than hard-coding indexes, we should use the pre-defined constants in defs.less. |
chipx86
|
|
|
Space after :. |
chipx86
|
|
|
Should just use 0 instead of 0px. No need for px when dealing with 0. |
chipx86
|
|
|
Space between these. |
chipx86
|
|
|
Any time you're dealing with precise values like these (-160px or the color), you should use constants. Also, don't use … |
chipx86
|
|
|
Should be more like #mobile-account-nav. |
chipx86
|
|
|
Space after :. Go through the rest of these and make sure they're in the right form. |
chipx86
|
|
|
Trailing whitespace. |
chipx86
|
|
|
Check other JavaScript files for the format needed for doc comments. |
chipx86
|
|
|
Space after if. |
chipx86
|
|
|
We should call this base/_nav_support_menu.html |
chipx86
|
|
|
We only use 1-space indentation for HTML. In this case, don't indent the template tag at all. |
chipx86
|
|
|
Only one blank line here. |
chipx86
|
|
|
Only 1-space indentation. |
chipx86
|
|
|
Can we call this search-icon? |
david
|
|
|
Spaces around the > chacacters. |
david
|
|
|
Space between ) and { |
david
|
|
|
Please break this up onto several lines, and prefer single quotes over double. $('#navbar-container').animate({ left: 0 }, function() { $(this).addClass('menu-active'); … |
david
|
|
|
These should all be on one line. |
david
|
|
|
You've got some trailing whitespace here. Please also add in punctuation and rewrap. |
david
|
|
|
Space between ) and { |
david
|
|
|
{ on the same line |
david
|
|
|
{ should be on the same line as the if, and } else { should all be one line. You … |
david
|
|
|
I don't want to overload the term "banner." We already have banners for drafts of reviews and review requests. I'd … |
chipx86
|
|
|
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. |
david
|
|
|
Please add this line back. |
david
|
|
|
Can you fix these comments? |
david
|
|
|
Wrap to 80 columns (expand out the function to be one line per statement). You've also got some bad indentation … |
david
|
|
|
It's really weird to use _.template() this way. |
david
|
|
|
No spaces inside (). |
david
|
|
|
Indentation is wacky. Please make sure there are no tab characters and everything is done with spaces. |
david
|
|
|
Indentation. |
david
|
|
|
Indentation. |
david
|
|
|
Space after :. Should also be '0px' as a string rather than just an integer. |
david
|
|
|
Missing semicolon. |
david
|
|
|
Space after :. Should also be '-160px' as a string rather than just an integer. |
david
|
|
|
Missing semicolon. |
david
|
|
|
No need to assign this to a variable, just do new RB.HeaderView(...); |
david
|
|
|
Please undo these changes. |
david
|
|
|
I think you could remove the second line of this comment. It doesn't really add anything. |
david
|
|
|
Because you pass the element in when creating this view, you don't need to specify it here. |
david
|
|
|
Add a space after 'if' |
david
|
|
|
Wrap to 80 columns. |
david
|
|
|
Single quotes. |
david
|
|
|
Single quotes. |
david
|
|
|
Single quotes. |
david
|
|
|
There should be no changes to common.js in this review request. |
david
|
|
|
Remove this blank line. |
david
|
|
|
Please add spaces around >. |
david
|
|
|
Your changes here seem to have regressed the visual appearance for desktop-size windows. Please go through and make sure that … |
david
|
JT
- Description:
-
added responsive for main menu and 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) - Testing Done:
-
~ 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) ~ - 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
- - Testing
- - 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
-
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 :)
-
I haven't looked at the code yet, just the screenshots. Ditto on what Christian said about the summary/description.
-
-
-
JT
- Description:
-
added responsive for main menu and 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) ~ - 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
-
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.
-
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.
-
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.
-
-
-
-
-
-
Any time you're dealing with precise values like these (
-160pxor 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.
-
-
-
-
-
-
-
-
-
JT
- Change Summary:
-
This change corrected mainly coding style issues raised by Christian and also banner resize / mobile menu close when window is resized as mentioned in David's comment
- Commit:
-
03f1379d4e5ed04226d2e1ea521551ddabf1f0fe8be09a28279800c03512f561898e843e6b5e724f
- Diff:
-
Revision 2 (+202 -34)
-
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
JT
- Summary:
-
added responsive for main menu and banner[WIP] adding responsive functionality for main menu and banner
- Description:
-
~ added responsive for main menu and banner
~ 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.
~ Changes added
~ 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.
+ + [WIP] - refactoring previously added code out from common.js
+ + //
+ 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
-
-
-
-
-
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.
-
-
-
-
-
{should be on the same line as the if, and} else {should all be one line.You can also do the
mobileMenuToggleas 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
-
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.
JT
- Description:
-
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.
~ [WIP] - refactoring previously added code out from common.js
~ [WIP] - Finding the correct place for banner backbone view and how to inject it to the page.
//
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 ~ - 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, with temporary residency under headerbar.html - Commit:
-
8be09a28279800c03512f561898e843e6b5e724f5b47a9e3c7a8ce0474bd83d01e7e2f58f25b280d
- Diff:
-
Revision 3 (+287 -35)
-
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
JT
- Summary:
-
[WIP] Responsive UI for main menu and bannerResponsive UI for main menu and banner
- Description:
-
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.
~ [WIP] - Finding the correct place for banner backbone view and how to inject it to the page.
~ //
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, with temporary residency under headerbar.html ~ - moved menu toggle logic into it's own backbone view - Commit:
-
5b47a9e3c7a8ce0474bd83d01e7e2f58f25b280dc9e471f22058fb8851689cf8ab2407f7081bc790
- Diff:
-
Revision 4 (+221 -36)
-
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
JT
- Change Summary:
-
fixed code styling issue according to David's comment
- Commit:
-
c9e471f22058fb8851689cf8ab2407f7081bc79043c763d4ea5692a0ea8b4af6c701825a6d3dcfed
- Diff:
-
Revision 5 (+229 -47)
-
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
JT
- Commit:
-
43c763d4ea5692a0ea8b4af6c701825a6d3dcfed8e99eeefc38fd87906eca549174eff24f347a42b
- Diff:
-
Revision 6 (+227 -47)
-
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
JT
- Commit:
-
8e99eeefc38fd87906eca549174eff24f347a42bd84cec525f8c01de6dbdfcf5427f34c2960393c1
- Diff:
-
Revision 7 (+226 -46)
-
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
-
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: 0to its style rules. - The overlay mask overlays on top of the header bar, shading out the menu button. Can you change the mobile
#headerbarpadding 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.
- The overlay mask seems to be left-indented about 0.5em. Please add
JT
- Commit:
-
d84cec525f8c01de6dbdfcf5427f34c2960393c14a2654872bc8e66485a73d5dbefe7a6aed624a4b
- Diff:
-
Revision 8 (+233 -49)
-
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
JT
- Commit:
-
4a2654872bc8e66485a73d5dbefe7a6aed624a4b20d6cc7d3346f07b1e34bf4fb2e117d241afb262
- Diff:
-
Revision 9 (+237 -49)
-
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
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