Improving Admin UI - New Styles, Dashboard, Sidebar and Branding header
Review Request #2449 — Created July 1, 2011 and submitted
Information | |
---|---|
vladikoff | |
Review Board | |
Reviewers | |
reviewboard | |
Improving the Admin UI GitHub branch location (with binaries): https://github.com/vladikoff/reviewboard/tree/Improving-AdminUI2-stage Dependency: https://github.com/vladikoff/djblets/commits/cache-key-method or http://reviews.reviewboard.org/r/2515/
Cross-browser tested, Different Screen Resolutions, Python 2.4 - 2.7
Description | From | Last Updated |
---|---|---|
These changes should be pulled out into a separate review request. |
|
|
django and djblets are both 3rd-party modules and should be grouped together. |
|
|
These should be prefixed with reviewboard. |
|
|
There's some alignment issues here. It looks like several of these are indented only 3 spaces instead of 4. |
|
|
Same here re: MEDIA_URL and MEDIA_SERIAL |
|
|
Remove this line. |
|
|
Remove this line. |
|
|
These should be prefixed with the reviewboard. module. |
|
|
This should be a docstring. |
|
|
This can be made a lot more pythonic: for i in range(day_total): counter_date = start_date + timedelta(days=i) dates_in_days.append(counter_date) req_array.append([ request_objects.filter(time_added__lte=counter_date).count(), … |
|
|
Please include "TODO" in this comment. |
|
|
You can initialize these directly: requests = { 'all_requests': review_requests, 'requests_by_day': req_array, 'change_descriptions': change_desc_unique, } ... |
|
|
You can initialize this all at once. |
|
|
The items in this list would be better as tuples instead of lists: ('admin/db/auth/user/add/' _('Add New')), also, add a space … |
|
|
This can be initialized in place. I see more below. Can you go through and change everywhere that you're creating … |
|
|
Space between // and the comment text. Same below. |
|
|
Indentation is kind of funky here. |
|
|
Is this always going to be seconds? That seems like a kind of crummy unit to use for uptime. |
|
|
These should all have ?{{MEDIA_SERIAL}} at the end. |
|
|
Please change this to be "Review Request Status" |
|
|
Can you include an explanation for why this is commented out? |
|
|
These need to be placed in alphabetical ordering. django.contrib... should be before django.core... |
|
|
Should be in alphabetical ordering - ie, at the top of the from imports. |
|
|
Straight imports like this should be put before from imports. Also, are you sure you need to import all of … |
|
|
I believe this needs to be indented four spaces. |
|
|
I believe this should go with the group of "from" imports below this block. |
|
|
one space after = |
|
|
No linebreak here |
|
|
A linebreak before this if block |
|
|
This might look better with the line break after "(activity_data),", as opposed to after "HttpResponse(" |
|
|
Like above, I think this from import should join the group of from imports below. |
|
|
I believe each import should be on it's own and in alphabetical, like: import datetime import time |
|
|
Need to be put in alphabetical ordering. |
|
|
I believe we tend to format imports so that the imported objects are tabbed to the same point - see … |
|
|
Another linebreak before the start of this function. |
|
|
This is a little difficult to read. Is it possible to indent in a way that makes it easier to … |
|
|
Should be 4 spaces indentation, and spaces after the commas. |
|
|
space before the + |
|
|
Another linebreak before this function definition |
|
|
Space before the + |
|
|
Another linebreak before this function definition. |
|
|
No linebreak required here. |
|
|
A space before the + |
|
|
Another linebreak before this function definition. |
|
|
No linebreak required here. |
|
|
Another linebreak before this function definition. |
|
|
Space after the commas. |
|
|
Another linebreak before this function definition. Same applies to the rest of this review request, so I won't mention it … |
|
|
Space before + sign |
|
|
Should this be a constant declared somewhere? |
|
|
no linebreak here. |
|
|
Only need a single linebreak here |
|
|
No linebreak here |
|
|
Space after # |
|
|
Is this line <= 80 chars? |
|
|
Only one space before "in", and there should be a linebreak before this for block. |
|
|
Hm, instead of all this "idx" work, I'd prefer: for unique_desc in change_desc_unique: unique_desc_array = [] unique_desc_array.append(...) unique_desc_array.append(...) change_desc_array.append(unique_desc_array). Or, … |
|
|
Only one space before in, and a linebreak before the for block. Same for below. |
|
|
Space after # |
|
|
Spaces after commas. |
|
|
The CSS params should be in alphabetical order, so: display: inline; padding: 0; position: relative; vertical-align: middle; Same applies below. |
|
|
Two space indentation here. |
|
|
Two space indentation. |
|
|
Two space indentation |
|
|
Two space indentation. Other instances below. |
|
|
Four space indentation. |
|
|
Space after , |
|
|
var before widgetBoxId |
|
|
Maybe we can move the {% if widget_icon %} just above , and the {% endif %} right below, just … |
|
|
No space before this tag. |
|
|
Needs to be in alphabetical order. |
|
|
No space just before this |
|
|
No linebreak required here. |
|
|
Alphabetical order, please |
|
|
Proper indentation for the and tags please. More instances below. |
|
|
Can these if / else / endifs in this section get formatted to make it clearer what's within the code … |
|
|
Is that four spaces for indenting? Not sure, but it seems like it's a bit more. |
|
|
This line could probably be made clearer: {% trans... %} |
|
|
Four spaces for indentation. |
|
|
No spaces on either side of 'reload' |
|
|
No linebreak between these. |
|
|
Can this be split up into more than 1 line for clarity? |
|
|
No space before the {% |
|
|
I think this block of HTML can be un-indented a few spaces... |
|
|
Two spaces, not one before these tags. |
|
|
Needs to be properly indented. |
|
|
No space before the { |
|
|
Should be "var y..." |
|
|
Should be "var input_range_end", and the code in this function needs to be properly indented. |
|
|
"Comments", "Reviews", etc, should be in translate functions. |
|
|
Imports must be alphabetical. |
|
|
This must be inside the function, in the form of: def foo(): """One-line summary Further multi-line description. """ code |
|
|
delete_widget_cache. We don't use CamelCase for functions. |
|
|
This should probably be a constant defined outside the function. In this case, you'd call it CACHED_WIDGETS. |
|
|
Space between parameters, after the comma. |
|
|
Space between parameters, after the comma. Overriding the function is bad. It would need a new name or it'll be … |
|
|
request is already defined above. |
|
|
Blank line between these. |
|
|
request is already defined above. |
|
|
if ('_popup' not in request.REQUEST or 'pop' not in request.REQUEST): |
|
|
Indentation is off, and you don't need the \ Why not just provide current_site_config in there? You shouldn't need to … |
|
|
request is already defined above. |
|
|
This is sort of weird. Maybe pull this out above and pass in as a var. |
|
|
request is used above. |
|
|
Alphabetical order. Can you go through the rest of your change and make sure imports are in alphabetical order? |
|
|
Two blank lines. |
|
|
So I'm not sure we should have this here. Instead, these should be webapi resources. |
|
|
Follow the doc format outlined above. Same with all other instances in your change. |
|
|
I'd suggest: state = request.GET.get('collapse', None) widget = ... if state and widget: |
|
|
widget_sets. Can you go through your diff and make sure all variables and functions are in this_style instead? |
|
|
Two blank lines. Can you go through and make sure that's the case in other files? This applies to anything … |
|
|
Must be reviewboard.attachments.models, and be grouped with the rest of the imports. Go through the rest of your change and … |
|
|
No need for the \ on these. It's covered by being in the {} |
|
|
4 space indentation. |
|
|
Should tack onto the same var list, instead of adding a new "var" statement. Maybe just "width"? |
|
|
This can be: $(".admin-extras") .width(widSpace) .masonry("reload"); |
|
|
Can you explicitly do $(document).ready(..) ? |
|
|
space indentation. |
|
|
No spaces inside the () |
|
|
GETs that modify state are dangerous. Should always be a POST. Ideally, this should use a proper webapi resource class, … |
|
|
Indentation is off. No need for that empty function. |
|
|
There's an extra space at the beginning. |
|
|
Space after "if" |
|
|
No blank line. |
|
|
Group this with the CSS above. |
|
|
Indentation is inconsistent. Make sure your HTML in this change is using 1 space indentation. |
|
|
You shouldn't embed styles. Instead, use a class and define the rule. |
|
|
Same here about the style. |
|
|
$(document).ready() |
|
|
No space before xaxis. |
|
|
Only one blank line needed. Indentation is inconsistent. |
|
|
Really should define #tooltip once so we don't have to fetch it each time. |
|
|
No blank line. |
|
|
Indentation is wrong. |
|
|
Shouldn't embed style. Instead, define a CSS rule. |
|
-
This is a pretty great start. After playing with it a bit, I have a few requests: - For the "Server Cache" link, if there's no cache backend configured (or no cache stats), it should link to the page to turn on caching. - It would be nice to have some kind of blank-slate state for when there are no repositories or groups set up. - Some of the links on the left-hand side are broken.
-
reviewboard/admin/forms.py (Diff revision 1) These changes should be pulled out into a separate review request.
-
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 1) django and djblets are both 3rd-party modules and should be grouped together.
-
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 1) These should be prefixed with reviewboard.
-
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 1) {{MEDIA_URL}} and {{MEDIA_SERIAL}} are already injected into template contexts via middleware. You shouldn't need to pass them in yourself.
-
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 1) There's some alignment issues here. It looks like several of these are indented only 3 spaces instead of 4.
-
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 1) Same here re: MEDIA_URL and MEDIA_SERIAL
-
-
-
reviewboard/admin/widgets.py (Diff revision 1) These should be prefixed with the reviewboard. module.
-
-
reviewboard/admin/widgets.py (Diff revision 1) This can be made a lot more pythonic: for i in range(day_total): counter_date = start_date + timedelta(days=i) dates_in_days.append(counter_date) req_array.append([ request_objects.filter(time_added__lte=counter_date).count(), counter_date, ]) It might also be nice to fill req_array with dicts instead of with lists, so that you can address the data by name instead of [0] and [1].
-
-
reviewboard/admin/widgets.py (Diff revision 1) You can initialize these directly: requests = { 'all_requests': review_requests, 'requests_by_day': req_array, 'change_descriptions': change_desc_unique, } ...
-
-
reviewboard/admin/widgets.py (Diff revision 1) The items in this list would be better as tuples instead of lists: ('admin/db/auth/user/add/' _('Add New')), also, add a space after the comma and / at the end of the URLs.
-
reviewboard/admin/widgets.py (Diff revision 1) This can be initialized in place. I see more below. Can you go through and change everywhere that you're creating a dict like this?
-
reviewboard/htdocs/media/rb/js/admin.js (Diff revision 1) Space between // and the comment text. Same below.
-
-
reviewboard/templates/admin/admin_widget.html (Diff revision 1) Here's where it might be nice to use a dict: <a href="{{action.url}" class="{{action.class}}">{{action.text}}</a>
-
reviewboard/templates/admin/cache_stats.html (Diff revision 1) Is this always going to be seconds? That seems like a kind of crummy unit to use for uptime.
-
reviewboard/templates/admin/dashboard.html (Diff revision 1) These should all have ?{{MEDIA_SERIAL}} at the end.
-
reviewboard/templates/admin/dashboard.html (Diff revision 1) Please change this to be "Review Request Status"
-
reviewboard/templates/admin/feed.html (Diff revision 1) Can you include an explanation for why this is commented out?
Change Summary:
Updated based on feedback, this branch is work in progress...
Description: |
|
---|
Change Summary:
Updated with a new header and more...
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 3 (+2683 -247)
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Screenshots: |
|
Change Summary:
Updated with the progress
Diff: |
Revision 4 (+2903 -251)
|
---|
Change Summary:
Added fixes and new screenshots.
Diff: |
Revision 5 (+2919 -251)
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Screenshots: |
|
-
Vlad: Thanks for your work! A few notes before - mostly stylistic. All the best, -Mike
-
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 5) These need to be placed in alphabetical ordering. django.contrib... should be before django.core...
-
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 5) Should be in alphabetical ordering - ie, at the top of the from imports.
-
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 5) Straight imports like this should be put before from imports. Also, are you sure you need to import all of reviewboard? Or can you from import the individual functions that you need?
-
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 5) I believe this needs to be indented four spaces.
-
reviewboard/admin/views.py (Diff revision 5) I believe this should go with the group of "from" imports below this block.
-
-
-
-
reviewboard/admin/views.py (Diff revision 5) This might look better with the line break after "(activity_data),", as opposed to after "HttpResponse("
-
reviewboard/admin/widgets.py (Diff revision 5) Like above, I think this from import should join the group of from imports below.
-
reviewboard/admin/widgets.py (Diff revision 5) I believe each import should be on it's own and in alphabetical, like: import datetime import time
-
-
reviewboard/admin/widgets.py (Diff revision 5) I believe we tend to format imports so that the imported objects are tabbed to the same point - see https://github.com/reviewboard/reviewboard/blob/94e5004b28d4e10d4d774822026b1756ec5e067d/reviewboard/webapi/resources.py#L44 for example.
-
-
reviewboard/admin/widgets.py (Diff revision 5) This is a little difficult to read. Is it possible to indent in a way that makes it easier to tell that "(now -datetime.timedelta(days=7)..." belongs within users.filter( for "now"?
-
reviewboard/admin/widgets.py (Diff revision 5) Should be 4 spaces indentation, and spaces after the commas.
-
-
-
-
-
-
-
-
-
-
-
reviewboard/admin/widgets.py (Diff revision 5) Another linebreak before this function definition. Same applies to the rest of this review request, so I won't mention it anymore.
-
-
-
-
-
-
-
-
reviewboard/admin/widgets.py (Diff revision 5) Only one space before "in", and there should be a linebreak before this for block.
-
reviewboard/admin/widgets.py (Diff revision 5) Hm, instead of all this "idx" work, I'd prefer: for unique_desc in change_desc_unique: unique_desc_array = [] unique_desc_array.append(...) unique_desc_array.append(...) change_desc_array.append(unique_desc_array). Or, even better, that unique_desc_array maybe should be a tuple. Same applies below.
-
-
reviewboard/admin/widgets.py (Diff revision 5) Only one space before in, and a linebreak before the for block. Same for below.
-
-
-
reviewboard/htdocs/media/rb/css/admin.css (Diff revision 5) The CSS params should be in alphabetical order, so: display: inline; padding: 0; position: relative; vertical-align: middle; Same applies below.
-
-
-
-
reviewboard/htdocs/media/rb/css/admin.css (Diff revision 5) Two space indentation. Other instances below.
-
-
reviewboard/htdocs/media/rb/js/admin.js (Diff revision 5) Javascript variables should always be declared with "var", so: var centerWidth... var docWidth...
-
-
-
reviewboard/templates/admin/admin_widget.html (Diff revision 5) Maybe we can move the {% if widget_icon %} just above <img src...>, and the {% endif %} right below, just to make things a little clearer.
-
-
-
reviewboard/templates/admin/dashboard.html (Diff revision 5) No space just before this <script> tag.
-
-
-
reviewboard/templates/admin/widgets/w-actions.html (Diff revision 5) Proper indentation for the <h1> and <div> tags please. More instances below.
-
reviewboard/templates/admin/widgets/w-actions.html (Diff revision 5) Can these if / else / endifs in this section get formatted to make it clearer what's within the code blocks? Like {% if something %} [Block] {% else %} [Block] {% endif %}
-
reviewboard/templates/admin/widgets/w-groups.html (Diff revision 5) Is that four spaces for indenting? Not sure, but it seems like it's a bit more.
-
reviewboard/templates/admin/widgets/w-groups.html (Diff revision 5) This line could probably be made clearer: <p class="no-result"> {% trans... %} <br /> <a href... </p>
-
-
reviewboard/templates/admin/widgets/w-news.html (Diff revision 5) No spaces on either side of 'reload'
-
reviewboard/templates/admin/widgets/w-recent-actions.html (Diff revision 5) No linebreak between these.
-
reviewboard/templates/admin/widgets/w-recent-actions.html (Diff revision 5) Can this be split up into more than 1 line for clarity?
-
-
reviewboard/templates/admin/widgets/w-server-cache.html (Diff revision 5) I think this block of HTML can be un-indented a few spaces...
-
reviewboard/templates/admin/widgets/w-stats-large.html (Diff revision 5) Two spaces, not one before these tags.
-
reviewboard/templates/admin/widgets/w-stats-large.html (Diff revision 5) Needs to be properly indented.
-
-
-
reviewboard/templates/admin/widgets/w-stats-large.html (Diff revision 5) Should be "var input_range_end", and the code in this function needs to be properly indented.
-
reviewboard/templates/admin/widgets/w-stats.html (Diff revision 5) "Comments", "Reviews", etc, should be in translate functions.
Change Summary:
Updated based on feedback and a couple more fixes.
Description: |
|
|||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+2947 -267)
|
Change Summary:
Updates and fixes, space fixes.
Diff: |
Revision 7 (+2947 -267)
|
---|
Change Summary:
Added new branch link with a squashed commit. Cleaned up some formatting and added bug fixes.
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 8 (+3028 -268) |
-
So there's a fair bit to fix up still. I'm focusing mostly on the styling, but we're going to have to get some webapi work in for the toggles and stuff, because doing an HTTP GET is unsafe, and we want a standard way of doing all this. There's a lot of style stuff. It boils down to: * Indentation fixes (it's really inconsistent in most files) * Import order/grouping * Variable/function naming (in Python, always use foo_bar instead of fooBar -- note that JavaScript is going to be in fooBar style) * Other various PEP8 style issues. I'll do another pass once this is updated.
-
-
reviewboard/admin/signals.py (Diff revision 8) This must be inside the function, in the form of: def foo(): """One-line summary Further multi-line description. """ code
-
reviewboard/admin/signals.py (Diff revision 8) delete_widget_cache. We don't use CamelCase for functions.
-
reviewboard/admin/signals.py (Diff revision 8) This should probably be a constant defined outside the function. In this case, you'd call it CACHED_WIDGETS.
-
-
reviewboard/admin/signals.py (Diff revision 8) Space between parameters, after the comma. Overriding the function is bad. It would need a new name or it'll be overwritten. If we're really just tying delete_widget_cache to a signal, let's just do that directly.
-
-
-
-
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 8) if ('_popup' not in request.REQUEST or 'pop' not in request.REQUEST):
-
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 8) Indentation is off, and you don't need the \ Why not just provide current_site_config in there? You shouldn't need to pull these out specifically.
-
-
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 8) This is sort of weird. Maybe pull this out above and pass in as a var.
-
-
reviewboard/admin/views.py (Diff revision 8) Alphabetical order. Can you go through the rest of your change and make sure imports are in alphabetical order?
-
-
reviewboard/admin/views.py (Diff revision 8) So I'm not sure we should have this here. Instead, these should be webapi resources.
-
reviewboard/admin/views.py (Diff revision 8) Follow the doc format outlined above. Same with all other instances in your change.
-
reviewboard/admin/views.py (Diff revision 8) I'd suggest: state = request.GET.get('collapse', None) widget = ... if state and widget:
-
reviewboard/admin/views.py (Diff revision 8) widget_sets. Can you go through your diff and make sure all variables and functions are in this_style instead?
-
reviewboard/admin/views.py (Diff revision 8) Two blank lines. Can you go through and make sure that's the case in other files? This applies to anything at indentation level 0.
-
reviewboard/admin/widgets.py (Diff revision 8) Must be reviewboard.attachments.models, and be grouped with the rest of the imports. Go through the rest of your change and make sure all imports are correct. They should also be grouped into three groups: Python built-in modules, 3rd-party models, and project models. Each with a blank line between.
-
reviewboard/admin/widgets.py (Diff revision 8) No need for the \ on these. It's covered by being in the {}
-
-
reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8) Should tack onto the same var list, instead of adding a new "var" statement. Maybe just "width"?
-
reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8) This can be: $(".admin-extras") .width(widSpace) .masonry("reload");
-
reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8) Can you explicitly do $(document).ready(..) ?
-
-
-
reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8) GETs that modify state are dangerous. Should always be a POST. Ideally, this should use a proper webapi resource class, and go through datastore.js. The API work I talk about can be done in another change. Let's get the other fixes taken care of.
-
reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8) Indentation is off. No need for that empty function.
-
-
-
-
-
reviewboard/templates/admin/settings.html (Diff revision 8) Indentation is inconsistent. Make sure your HTML in this change is using 1 space indentation.
-
reviewboard/templates/admin/widgets/w-groups.html (Diff revision 8) You shouldn't embed styles. Instead, use a class and define the rule.
-
-
-
-
reviewboard/templates/admin/widgets/w-stats-large.html (Diff revision 8) Only one blank line needed. Indentation is inconsistent.
-
reviewboard/templates/admin/widgets/w-stats-large.html (Diff revision 8) Should be: $("<div id='tooltip'/>") .html(contents) .css({ ... }) .appendTo("body") .fadeIn(200);
-
reviewboard/templates/admin/widgets/w-stats-large.html (Diff revision 8) Really should define #tooltip once so we don't have to fetch it each time.
-
-
-
-
reviewboard/templates/admin/widgets/w-stats.html (Diff revision 8) Shouldn't embed style. Instead, define a CSS rule.