Summary

Responsive UI for main menu and banner

Review Request #6796 — Created Jan. 17, 2015 and submitted April 13, 2015, 4:37 p.m.

Information

Owner

jtsengyc

Repository

Review Board

Branch

master

Bugs

Depends On

Commit

20d6cc7...

Reviewers

Groups

reviewboard, students

People

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.

//
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

Testing Done


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

Files

Issues

Description	From	Last Updated
Can you stretch the menu vertically to fill the entire height of the document?	david	Jan. 18, 2015, 11:13 a.m.
Can we remove the dimming when the window gets larger again?	david	Jan. 22, 2015, 3:35 p.m.
How about scaling down this icon so that the entire header is the height of the search icon?	david	Jan. 22, 2015, 4:28 p.m.
Can we reduce the size of this so that everything is the height of the text / search icon?	david	March 13, 2015, 11:07 p.m.
This would be better in defs.less. However, we probably want to be more specific in the name, because there's a …	chipx86	Jan. 18, 2015, 11:43 a.m.
Space after the :. Given that we're doing this media query a lot, we should have a macro for it …	chipx86	Feb. 2, 2015, 12:19 a.m.
Trailing whitespace. Should use .opacity(0.5);	chipx86	Jan. 18, 2015, 11:43 a.m.
Rather than hard-coding indexes, we should use the pre-defined constants in defs.less.	chipx86	Jan. 18, 2015, 11:43 a.m.
Space after :.	chipx86	Jan. 18, 2015, 11:43 a.m.
Should just use 0 instead of 0px. No need for px when dealing with 0.	chipx86	Jan. 18, 2015, 11:43 a.m.
Space between these.	chipx86	Jan. 18, 2015, 11:43 a.m.
Any time you're dealing with precise values like these (-160px or the color), you should use constants. Also, don't use …	chipx86	Jan. 18, 2015, 11:56 a.m.
Should be more like #mobile-account-nav.	chipx86	Jan. 18, 2015, 11:49 a.m.
Space after :. Go through the rest of these and make sure they're in the right form.	chipx86	Jan. 18, 2015, 11:47 a.m.
Trailing whitespace.	chipx86	Jan. 18, 2015, 11:56 a.m.
Check other JavaScript files for the format needed for doc comments.	chipx86	Jan. 18, 2015, 12:08 p.m.
Space after if.	chipx86	Jan. 18, 2015, 11:54 a.m.
We should call this base/_nav_support_menu.html	chipx86	Jan. 18, 2015, 11:54 a.m.
We only use 1-space indentation for HTML. In this case, don't indent the template tag at all.	chipx86	Jan. 18, 2015, 11:54 a.m.
Only one blank line here.	chipx86	Jan. 18, 2015, 11:54 a.m.
Only 1-space indentation.	chipx86	Jan. 18, 2015, 11:54 a.m.
Can we call this search-icon?	david	Feb. 21, 2015, 11:35 p.m.
Spaces around the > chacacters.	david	Feb. 21, 2015, 11:35 p.m.
Space between ) and {	david	Feb. 21, 2015, 11:35 p.m.
Please break this up onto several lines, and prefer single quotes over double. $('#navbar-container').animate({ left: 0 }, function() { $(this).addClass('menu-active'); …	david	Feb. 21, 2015, 11:35 p.m.
These should all be on one line.	david	Feb. 21, 2015, 11:36 p.m.
You've got some trailing whitespace here. Please also add in punctuation and rewrap.	david	Feb. 21, 2015, 11:36 p.m.
Space between ) and {	david	Feb. 21, 2015, 11:39 p.m.
{ on the same line	david	Feb. 21, 2015, 11:38 p.m.
{ should be on the same line as the if, and } else { should all be one line. You …	david	Feb. 21, 2015, 11:39 p.m.
I don't want to overload the term "banner." We already have banners for drafts of reviews and review requests. I'd …	chipx86	Feb. 23, 2015, 1:17 a.m.
These are indented via tabs, as are some of the other lines (31, 39, 43), should be spaces. :/	SU Sunxperous	Feb. 22, 2015, 8:31 a.m.
Same tabs and spaces identation issue for this part.	SU Sunxperous	Feb. 22, 2015, 8:31 a.m.
Indentation in LESS files should be 2 spaces.	david	March 3, 2015, 12:52 a.m.
Please add this line back.	david	March 3, 2015, 12:52 a.m.
Can you fix these comments?	david	March 3, 2015, 12:53 a.m.
Wrap to 80 columns (expand out the function to be one line per statement). You've also got some bad indentation …	david	March 3, 2015, 1:16 a.m.
It's really weird to use _.template() this way.	david	March 3, 2015, 1:16 a.m.
No spaces inside ().	david	March 3, 2015, 12:55 a.m.
Indentation is wacky. Please make sure there are no tab characters and everything is done with spaces.	david	March 3, 2015, 1:01 a.m.
Indentation.	david	March 3, 2015, 12:58 a.m.
Indentation.	david	March 3, 2015, 12:58 a.m.
Space after :. Should also be '0px' as a string rather than just an integer.	david	March 3, 2015, 1 a.m.
Missing semicolon.	david	March 3, 2015, 1:01 a.m.
Space after :. Should also be '-160px' as a string rather than just an integer.	david	March 3, 2015, 1:01 a.m.
Missing semicolon.	david	March 3, 2015, 1:01 a.m.
No need to assign this to a variable, just do new RB.HeaderView(...);	david	March 3, 2015, 1:01 a.m.
Please undo these changes.	david	March 13, 2015, 11:07 p.m.
I think you could remove the second line of this comment. It doesn't really add anything.	david	March 9, 2015, 10:54 a.m.
Because you pass the element in when creating this view, you don't need to specify it here.	david	March 9, 2015, 10:54 a.m.
Add a space after 'if'	david	March 9, 2015, 10:55 a.m.
Wrap to 80 columns.	david	March 9, 2015, 10:56 a.m.
Single quotes.	david	March 9, 2015, 10:56 a.m.
Single quotes.	david	March 9, 2015, 10:56 a.m.
Single quotes.	david	March 9, 2015, 10:56 a.m.
There should be no changes to common.js in this review request.	david	March 16, 2015, 2:42 p.m.
Remove this blank line.	david	March 16, 2015, 2:42 p.m.
Please add spaces around >.	david	March 16, 2015, 2:42 p.m.
Your changes here seem to have regressed the visual appearance for desktop-size windows. Please go through and make sure that …	david	March 26, 2015, 6:45 p.m.

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

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

When making UI changes, please add lots of screenshots as file attachments.

Added Files:

Added Files:

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.

mobile view menu opened (Revision 1)

The issue has been resolved. Show all issues

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

resizing to > 720px after mobile menu opened (Revision 1)

The issue has been resolved. Show all issues

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

jtsengyc Jan. 17, 2015, 11:20 a.m.

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?

david Jan. 17, 2015, 11:52 a.m.

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.

mobile view menu opened (Revision 1)

The issue has been resolved. Show all issues

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

jtsengyc Jan. 17, 2015, 11:20 a.m.

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

david Jan. 17, 2015, 11:52 a.m.

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

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.

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

The issue has been resolved. Show all issues

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.

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

The issue has been resolved. Show all issues

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.

jtsengyc Jan. 22, 2015, 4:29 p.m.

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

reviewboard/static/rb/css/pages/base.less (Diff revision 1)
The issue has been resolved. Show all issues
```
Trailing whitespace.
Should use .opacity(0.5);
```
reviewboard/static/rb/css/pages/base.less (Diff revision 1)
The issue has been resolved. Show all issues
```
Rather than hard-coding indexes, we should use the pre-defined constants in defs.less.
```
reviewboard/static/rb/css/pages/base.less (Diff revision 1)
The issue has been resolved. Show all issues
```
Space after :.
```
reviewboard/static/rb/css/pages/base.less (Diff revision 1)
The issue has been resolved. Show all issues
```
Should just use 0 instead of 0px. No need for px when dealing with 0.
```
reviewboard/static/rb/css/pages/base.less (Diff revision 1)
The issue has been resolved. Show all issues
```
Space between these.
```

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

The issue has been resolved. Show all issues

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.

reviewboard/static/rb/css/pages/base.less (Diff revision 1)
The issue has been resolved. Show all issues
```
Should be more like #mobile-account-nav.
```
reviewboard/static/rb/css/pages/base.less (Diff revision 1)
The issue has been resolved. Show all issues
```
Space after :.
Go through the rest of these and make sure they're in the right form.
```
reviewboard/static/rb/css/pages/base.less (Diff revision 1)
The issue has been resolved. Show all issues
```
Trailing whitespace.
```
reviewboard/static/rb/js/common.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Check other JavaScript files for the format needed for doc comments.
```
reviewboard/static/rb/js/common.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Space after if.
```
reviewboard/templates/base/headerbar.html (Diff revision 1)
The issue has been resolved. Show all issues
```
We should call this base/_nav_support_menu.html
```

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

The issue has been resolved. Show all issues

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

reviewboard/templates/base/navsupportmenu.html (Diff revision 1)
The issue has been resolved. Show all issues
```
Only one blank line here.
```
reviewboard/templates/base/navsupportmenu.html (Diff revision 1)
The issue has been resolved. Show all issues
```
Only 1-space indentation.
```

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:

-	03f1379d4e5ed04226d2e1ea521551ddabf1f0fe
+	8be09a28279800c03512f561898e843e6b5e724f

Diff:

Revision 2 (+202 -34)

Show changes

	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/base.less
	reviewboard/static/rb/js/common.js
	reviewboard/templates/base.html
	reviewboard/templates/base/_nav_support_menu.html
	reviewboard/templates/base/branding.html
	reviewboard/templates/base/headerbar.html
	reviewboard/templates/base/navbar.html

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

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

reviewboard/static/rb/css/pages/base.less (Diff revision 2)
The issue has been resolved. Show all issues
```
Can we call this search-icon?
```
reviewboard/static/rb/css/pages/base.less (Diff revision 2)
The issue has been resolved. Show all issues
```
Spaces around the > chacacters.
```
reviewboard/static/rb/js/common.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Space between ) and {
```

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

The issue has been resolved. Show all issues

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.

reviewboard/static/rb/js/common.js (Diff revision 2)
The issue has been resolved. Show all issues
```
These should all be on one line.
```

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

The issue has been resolved. Show all issues

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

reviewboard/static/rb/js/common.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Space between ) and {
```
reviewboard/static/rb/js/common.js (Diff revision 2)
The issue has been resolved. Show all issues
```
{ on the same line
```

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

The issue has been resolved. Show all issues

{ 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

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.

Summary:

-	[WIP] adding responsive functionality for main menu and banner
+	[WIP] Responsive 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] - 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:

-	8be09a28279800c03512f561898e843e6b5e724f
+	5b47a9e3c7a8ce0474bd83d01e7e2f58f25b280d

Diff:

Revision 3 (+287 -35)

Show changes

	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/base.less
	reviewboard/static/rb/js/common.js
	reviewboard/static/rb/js/views/bannerView.js
	reviewboard/templates/base/_nav_support_menu.html
	reviewboard/templates/base/branding.html
	reviewboard/templates/base/headerbar.html
	reviewboard/templates/base/navbar.html

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

jtsengyc Feb. 22, 2015, 8:34 a.m.

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.

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

The issue has been dropped. Show all issues

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

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

The issue has been dropped. Show all issues

Same tabs and spaces identation issue for this part.

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

The issue has been resolved. Show all issues

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.

david Feb. 22, 2015, 2:36 p.m.
```
Probably should be "header".
```

Summary:

-	[WIP] Responsive UI for main menu and banner
+	Responsive 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:

-	5b47a9e3c7a8ce0474bd83d01e7e2f58f25b280d
+	c9e471f22058fb8851689cf8ab2407f7081bc790

Diff:

Revision 4 (+221 -36)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/base.less
	reviewboard/static/rb/js/common.js
	reviewboard/static/rb/js/views/headerView.js
	reviewboard/templates/base.html
	reviewboard/templates/base/_nav_support_menu.html
	reviewboard/templates/base/branding.html
	2 more

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

reviewboard/static/rb/css/pages/base.less (Diff revision 4)
The issue has been resolved. Show all issues
```
Indentation in LESS files should be 2 spaces.
```
reviewboard/static/rb/js/common.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Please add this line back.
```
reviewboard/static/rb/js/views/headerView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Can you fix these comments?
```

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

The issue has been resolved. Show all issues

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

reviewboard/static/rb/js/views/headerView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
It's really weird to use _.template() this way.
```
reviewboard/static/rb/js/views/headerView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
No spaces inside ().
```

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

The issue has been resolved. Show all issues

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

reviewboard/static/rb/js/views/headerView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Indentation.
```
reviewboard/static/rb/js/views/headerView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Indentation.
```
reviewboard/static/rb/js/views/headerView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Space after :. Should also be '0px' as a string rather than just an integer.
```
reviewboard/static/rb/js/views/headerView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Missing semicolon.
```
reviewboard/static/rb/js/views/headerView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Space after :. Should also be '-160px' as a string rather than just an integer.
```
reviewboard/static/rb/js/views/headerView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Missing semicolon.
```
reviewboard/templates/base.html (Diff revision 4)
The issue has been resolved. Show all issues
```
No need to assign this to a variable, just do new RB.HeaderView(...);
```

Change Summary:

fixed code styling issue according to David's comment

Commit:

-	c9e471f22058fb8851689cf8ab2407f7081bc790
+	43c763d4ea5692a0ea8b4af6c701825a6d3dcfed

Diff:

Revision 5 (+229 -47)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/base.less
	reviewboard/static/rb/js/common.js
	reviewboard/static/rb/js/views/headerView.js
	reviewboard/templates/base.html
	reviewboard/templates/base/_nav_support_menu.html
	reviewboard/templates/base/branding.html
	2 more

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

mobile view (Revision 1)

The issue has been resolved. Show all issues

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

jtsengyc March 9, 2015, 11:06 a.m.

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

david March 13, 2015, 1:09 p.m.

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

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

The issue has been resolved. Show all issues

Please undo these changes.

jtsengyc March 9, 2015, 11:06 a.m.

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

david March 13, 2015, 1:09 p.m.

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.

reviewboard/static/rb/js/views/headerView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
I think you could remove the second line of this comment. It doesn't really add anything.
```
reviewboard/static/rb/js/views/headerView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Because you pass the element in when creating this view, you don't need to specify it here.
```
reviewboard/static/rb/js/views/headerView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Add a space after 'if'
```
reviewboard/static/rb/js/views/headerView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Wrap to 80 columns.
```
reviewboard/static/rb/js/views/headerView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Single quotes.
```
reviewboard/static/rb/js/views/headerView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Single quotes.
```
reviewboard/templates/base.html (Diff revision 5)
The issue has been resolved. Show all issues
```
Single quotes.
```

Added Files:

Commit:

-	43c763d4ea5692a0ea8b4af6c701825a6d3dcfed
+	8e99eeefc38fd87906eca549174eff24f347a42b

Diff:

Revision 6 (+227 -47)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/base.less
	reviewboard/static/rb/js/common.js
	reviewboard/static/rb/js/views/headerView.js
	reviewboard/templates/base.html
	reviewboard/templates/base/_nav_support_menu.html
	reviewboard/templates/base/branding.html
	2 more

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

reviewboard/static/rb/js/common.js (Diff revision 6)
The issue has been resolved. Show all issues
```
There should be no changes to common.js in this review request.
```
reviewboard/static/rb/js/views/headerView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Remove this blank line.
```
reviewboard/static/rb/js/views/headerView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Please add spaces around >.
```

Commit:

-	8e99eeefc38fd87906eca549174eff24f347a42b
+	d84cec525f8c01de6dbdfcf5427f34c2960393c1

Diff:

Revision 7 (+226 -46)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/base.less
	reviewboard/static/rb/js/views/headerView.js
	reviewboard/templates/base.html
	reviewboard/templates/base/_nav_support_menu.html
	reviewboard/templates/base/branding.html
	reviewboard/templates/base/headerbar.html
	1 more

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: 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.

jtsengyc March 19, 2015, 7:45 p.m.

Reviewed

ok
ok
ok - added 10px margin
the <br> element was there originally, is that something we want to remove?
ok - added 10px margin too

david March 23, 2015, 3:38 p.m.

For 4, I think yes, we would want to remove it.

Commit:

-	d84cec525f8c01de6dbdfcf5427f34c2960393c1
+	4a2654872bc8e66485a73d5dbefe7a6aed624a4b

Diff:

Revision 8 (+233 -49)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/base.less
	reviewboard/static/rb/js/views/headerView.js
	reviewboard/templates/base.html
	reviewboard/templates/base/_nav_support_menu.html
	reviewboard/templates/base/branding.html
	reviewboard/templates/base/headerbar.html
	1 more

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

reviewboard/static/rb/css/pages/base.less (Diff revisions 7 - 8)

The issue has been resolved. Show all issues

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.

jtsengyc March 26, 2015, 6:46 p.m.

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. :)

david March 27, 2015, 1:57 p.m.

I'm not sure I understand what you mean.

Is there a new revision that I can review?

jtsengyc March 27, 2015, 2:43 p.m.

There are a few more currently listed under [WIP] linked below, should I remove the [WIP]?
6922, #6893, #6923, #6952, #6954

Commit:

-	4a2654872bc8e66485a73d5dbefe7a6aed624a4b
+	20d6cc7d3346f07b1e34bf4fb2e117d241afb262

Diff:

Revision 9 (+237 -49)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/base.less
	reviewboard/static/rb/js/views/headerView.js
	reviewboard/templates/base.html
	reviewboard/templates/base/_nav_support_menu.html
	reviewboard/templates/base/branding.html
	reviewboard/templates/base/headerbar.html
	1 more

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

Ship it!

I'm going to make some simple changes to fix up some small issues, and then push this. Thanks!

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (618f8fc)

Responsive UI for main menu and banner

Information

Reviewers

Description:

Testing Done:

Added Files:

Added Files:

Description:

Change Summary:

Commit:

Diff:

Summary:

Description:

Summary:

Description:

Commit:

Diff:

Summary:

Description:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Added Files:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

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

Commit:

Diff:

Status: Closed (submitted)

Change Summary: