Improving Admin UI - New Styles, Dashboard, Sidebar and Branding header

Review Request #2449 — Created July 1, 2011 and submitted

Information

Review Board

Reviewers

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 

Screenshots

Description From Last Updated

These changes should be pulled out into a separate review request.

daviddavid

django and djblets are both 3rd-party modules and should be grouped together.

daviddavid

These should be prefixed with reviewboard.

daviddavid

There's some alignment issues here. It looks like several of these are indented only 3 spaces instead of 4.

daviddavid

Same here re: MEDIA_URL and MEDIA_SERIAL

daviddavid

Remove this line.

daviddavid

Remove this line.

daviddavid

These should be prefixed with the reviewboard. module.

daviddavid

This should be a docstring.

daviddavid

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(), …

daviddavid

Please include "TODO" in this comment.

daviddavid

You can initialize these directly: requests = { 'all_requests': review_requests, 'requests_by_day': req_array, 'change_descriptions': change_desc_unique, } ...

daviddavid

You can initialize this all at once.

daviddavid

The items in this list would be better as tuples instead of lists: ('admin/db/auth/user/add/' _('Add New')), also, add a space …

daviddavid

This can be initialized in place. I see more below. Can you go through and change everywhere that you're creating …

daviddavid

Space between // and the comment text. Same below.

daviddavid

Indentation is kind of funky here.

daviddavid

Is this always going to be seconds? That seems like a kind of crummy unit to use for uptime.

daviddavid

These should all have ?{{MEDIA_SERIAL}} at the end.

daviddavid

Please change this to be "Review Request Status"

daviddavid

Can you include an explanation for why this is commented out?

daviddavid

These need to be placed in alphabetical ordering. django.contrib... should be before django.core...

mike_conleymike_conley

Should be in alphabetical ordering - ie, at the top of the from imports.

mike_conleymike_conley

Straight imports like this should be put before from imports. Also, are you sure you need to import all of …

mike_conleymike_conley

I believe this needs to be indented four spaces.

mike_conleymike_conley

I believe this should go with the group of "from" imports below this block.

mike_conleymike_conley

one space after =

mike_conleymike_conley

No linebreak here

mike_conleymike_conley

A linebreak before this if block

mike_conleymike_conley

This might look better with the line break after "(activity_data),", as opposed to after "HttpResponse("

mike_conleymike_conley

Like above, I think this from import should join the group of from imports below.

mike_conleymike_conley

I believe each import should be on it's own and in alphabetical, like: import datetime import time

mike_conleymike_conley

Need to be put in alphabetical ordering.

mike_conleymike_conley

I believe we tend to format imports so that the imported objects are tabbed to the same point - see …

mike_conleymike_conley

Another linebreak before the start of this function.

mike_conleymike_conley

This is a little difficult to read. Is it possible to indent in a way that makes it easier to …

mike_conleymike_conley

Should be 4 spaces indentation, and spaces after the commas.

mike_conleymike_conley

space before the +

mike_conleymike_conley

Another linebreak before this function definition

mike_conleymike_conley

Space before the +

mike_conleymike_conley

Another linebreak before this function definition.

mike_conleymike_conley

No linebreak required here.

mike_conleymike_conley

A space before the +

mike_conleymike_conley

Another linebreak before this function definition.

mike_conleymike_conley

No linebreak required here.

mike_conleymike_conley

Another linebreak before this function definition.

mike_conleymike_conley

Space after the commas.

mike_conleymike_conley

Another linebreak before this function definition. Same applies to the rest of this review request, so I won't mention it …

mike_conleymike_conley

Space before + sign

mike_conleymike_conley

Should this be a constant declared somewhere?

mike_conleymike_conley

no linebreak here.

mike_conleymike_conley

Only need a single linebreak here

mike_conleymike_conley

No linebreak here

mike_conleymike_conley

Space after #

mike_conleymike_conley

Is this line <= 80 chars?

mike_conleymike_conley

Only one space before "in", and there should be a linebreak before this for block.

mike_conleymike_conley

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, …

mike_conleymike_conley

Only one space before in, and a linebreak before the for block. Same for below.

mike_conleymike_conley

Space after #

mike_conleymike_conley

Spaces after commas.

mike_conleymike_conley

The CSS params should be in alphabetical order, so: display: inline; padding: 0; position: relative; vertical-align: middle; Same applies below.

mike_conleymike_conley

Two space indentation here.

mike_conleymike_conley

Two space indentation.

mike_conleymike_conley

Two space indentation

mike_conleymike_conley

Two space indentation. Other instances below.

mike_conleymike_conley

Four space indentation.

mike_conleymike_conley

Space after ,

mike_conleymike_conley

var before widgetBoxId

mike_conleymike_conley

Maybe we can move the {% if widget_icon %} just above , and the {% endif %} right below, just …

mike_conleymike_conley

No space before this tag.

mike_conleymike_conley

Needs to be in alphabetical order.

mike_conleymike_conley

No space just before this

mike_conleymike_conley

No linebreak required here.

mike_conleymike_conley

Alphabetical order, please

mike_conleymike_conley

Proper indentation for the and tags please. More instances below.

mike_conleymike_conley

Can these if / else / endifs in this section get formatted to make it clearer what's within the code …

mike_conleymike_conley

Is that four spaces for indenting? Not sure, but it seems like it's a bit more.

mike_conleymike_conley

This line could probably be made clearer: {% trans... %}

mike_conleymike_conley

Four spaces for indentation.

mike_conleymike_conley

No spaces on either side of 'reload'

mike_conleymike_conley

No linebreak between these.

mike_conleymike_conley

Can this be split up into more than 1 line for clarity?

mike_conleymike_conley

No space before the {%

mike_conleymike_conley

I think this block of HTML can be un-indented a few spaces...

mike_conleymike_conley

Two spaces, not one before these tags.

mike_conleymike_conley

Needs to be properly indented.

mike_conleymike_conley

No space before the {

mike_conleymike_conley

Should be "var y..."

mike_conleymike_conley

Should be "var input_range_end", and the code in this function needs to be properly indented.

mike_conleymike_conley

"Comments", "Reviews", etc, should be in translate functions.

mike_conleymike_conley

Imports must be alphabetical.

chipx86chipx86

This must be inside the function, in the form of: def foo(): """One-line summary Further multi-line description. """ code

chipx86chipx86

delete_widget_cache. We don't use CamelCase for functions.

chipx86chipx86

This should probably be a constant defined outside the function. In this case, you'd call it CACHED_WIDGETS.

chipx86chipx86

Space between parameters, after the comma.

chipx86chipx86

Space between parameters, after the comma. Overriding the function is bad. It would need a new name or it'll be …

chipx86chipx86

request is already defined above.

chipx86chipx86

Blank line between these.

chipx86chipx86

request is already defined above.

chipx86chipx86

if ('_popup' not in request.REQUEST or 'pop' not in request.REQUEST):

chipx86chipx86

Indentation is off, and you don't need the \ Why not just provide current_site_config in there? You shouldn't need to …

chipx86chipx86

request is already defined above.

chipx86chipx86

This is sort of weird. Maybe pull this out above and pass in as a var.

chipx86chipx86

request is used above.

chipx86chipx86

Alphabetical order. Can you go through the rest of your change and make sure imports are in alphabetical order?

chipx86chipx86

Two blank lines.

chipx86chipx86

So I'm not sure we should have this here. Instead, these should be webapi resources.

chipx86chipx86

Follow the doc format outlined above. Same with all other instances in your change.

chipx86chipx86

I'd suggest: state = request.GET.get('collapse', None) widget = ... if state and widget:

chipx86chipx86

widget_sets. Can you go through your diff and make sure all variables and functions are in this_style instead?

chipx86chipx86

Two blank lines. Can you go through and make sure that's the case in other files? This applies to anything …

chipx86chipx86

Must be reviewboard.attachments.models, and be grouped with the rest of the imports. Go through the rest of your change and …

chipx86chipx86

No need for the \ on these. It's covered by being in the {}

chipx86chipx86

4 space indentation.

chipx86chipx86

Should tack onto the same var list, instead of adding a new "var" statement. Maybe just "width"?

chipx86chipx86

This can be: $(".admin-extras") .width(widSpace) .masonry("reload");

chipx86chipx86

Can you explicitly do $(document).ready(..) ?

chipx86chipx86

space indentation.

chipx86chipx86

No spaces inside the ()

chipx86chipx86

GETs that modify state are dangerous. Should always be a POST. Ideally, this should use a proper webapi resource class, …

chipx86chipx86

Indentation is off. No need for that empty function.

chipx86chipx86

There's an extra space at the beginning.

chipx86chipx86

Space after "if"

chipx86chipx86

No blank line.

chipx86chipx86

Group this with the CSS above.

chipx86chipx86

Indentation is inconsistent. Make sure your HTML in this change is using 1 space indentation.

chipx86chipx86

You shouldn't embed styles. Instead, use a class and define the rule.

chipx86chipx86

Same here about the style.

chipx86chipx86

$(document).ready()

chipx86chipx86

No space before xaxis.

chipx86chipx86

Only one blank line needed. Indentation is inconsistent.

chipx86chipx86

Really should define #tooltip once so we don't have to fetch it each time.

chipx86chipx86

No blank line.

chipx86chipx86

Indentation is wrong.

chipx86chipx86

Shouldn't embed style. Instead, define a CSS rule.

chipx86chipx86
david
  1. Can you make this available as a branch on your github fork? I applied the patch but it's missing all the binary files.
  2. 
      
david
  1. 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.
  2. reviewboard/admin/forms.py (Diff revision 1)
     
     
     
     
     
     
    Show all issues
    These changes should be pulled out into a separate review request.
  3. reviewboard/admin/templatetags/rbadmintags.py (Diff revision 1)
     
     
     
     
     
    Show all issues
    django and djblets are both 3rd-party modules and should be grouped together.
  4. reviewboard/admin/templatetags/rbadmintags.py (Diff revision 1)
     
     
     
     
    Show all issues
    These should be prefixed with reviewboard.
  5. {{MEDIA_URL}} and {{MEDIA_SERIAL}} are already injected into template contexts via middleware. You shouldn't need to pass them in yourself.
  6. reviewboard/admin/templatetags/rbadmintags.py (Diff revision 1)
     
     
     
     
    Show all issues
    There's some alignment issues here. It looks like several of these are indented only 3 spaces instead of 4.
  7. Show all issues
    Same here re: MEDIA_URL and MEDIA_SERIAL
  8. reviewboard/admin/views.py (Diff revision 1)
     
     
    Show all issues
    Remove this line.
  9. reviewboard/admin/views.py (Diff revision 1)
     
     
    Show all issues
    Remove this line.
  10. reviewboard/admin/widgets.py (Diff revision 1)
     
     
     
     
    Show all issues
    These should be prefixed with the reviewboard. module.
  11. reviewboard/admin/widgets.py (Diff revision 1)
     
     
     
    Show all issues
    This should be a docstring.
  12. reviewboard/admin/widgets.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    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].
  13. reviewboard/admin/widgets.py (Diff revision 1)
     
     
    Show all issues
    Please include "TODO" in this comment.
  14. reviewboard/admin/widgets.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    You can initialize these directly:
    
    requests = {
        'all_requests': review_requests,
        'requests_by_day': req_array,
        'change_descriptions': change_desc_unique,
    }
    
    ...
  15. reviewboard/admin/widgets.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    You can initialize this all at once.
  16. reviewboard/admin/widgets.py (Diff revision 1)
     
     
     
     
     
    Show all issues
    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.
  17. reviewboard/admin/widgets.py (Diff revision 1)
     
     
     
     
     
     
    Show all issues
    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?
  18. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 1)
     
     
    Show all issues
    Space between // and the comment text. Same below.
  19. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Indentation is kind of funky here.
  20. Here's where it might be nice to use a dict:
    
    <a href="{{action.url}" class="{{action.class}}">{{action.text}}</a>
  21. Show all issues
    Is this always going to be seconds? That seems like a kind of crummy unit to use for uptime.
  22. reviewboard/templates/admin/dashboard.html (Diff revision 1)
     
     
     
     
     
     
    Show all issues
    These should all have ?{{MEDIA_SERIAL}} at the end.
  23. Show all issues
    Please change this to be "Review Request Status"
  24. reviewboard/templates/admin/feed.html (Diff revision 1)
     
     
     
     
    Show all issues
    Can you include an explanation for why this is commented out?
  25. 
      
VL
VL
VL
chipx86
  1. 
      
  2. This is weirdly yellow. What happened?
  3. 
      
VL
VL
mike_conley
  1. Vlad:
    
    Thanks for your work!  A few notes before - mostly stylistic.
    
    All the best,
    
    -Mike
  2. reviewboard/admin/templatetags/rbadmintags.py (Diff revision 5)
     
     
     
     
    Show all issues
    These need to be placed in alphabetical ordering.  django.contrib... should be before django.core...
  3. Show all issues
    Should be in alphabetical ordering - ie, at the top of the from imports.
  4. Show all issues
    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?
  5. Show all issues
    I believe this needs to be indented four spaces.
  6. reviewboard/admin/views.py (Diff revision 5)
     
     
    Show all issues
    I believe this should go with the group of "from" imports below this block.
  7. reviewboard/admin/views.py (Diff revision 5)
     
     
    Show all issues
    one space after =
  8. reviewboard/admin/views.py (Diff revision 5)
     
     
    Show all issues
    No linebreak here
  9. reviewboard/admin/views.py (Diff revision 5)
     
     
    Show all issues
    A linebreak before this if block
  10. reviewboard/admin/views.py (Diff revision 5)
     
     
     
    Show all issues
    This might look better with the line break after "(activity_data),", as opposed to after "HttpResponse("
  11. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues
    Like above, I think this from import should join the group of from imports below.
  12. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues
    I believe each import should be on it's own and in alphabetical, like:
    
    import datetime
    import time
  13. reviewboard/admin/widgets.py (Diff revision 5)
     
     
     
    Show all issues
    Need to be put in alphabetical ordering.
  14. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues
    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.
  15. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues
    Another linebreak before the start of this function.
  16. reviewboard/admin/widgets.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
    Show all issues
    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"?
  17. reviewboard/admin/widgets.py (Diff revision 5)
     
     
     
    Show all issues
    Should be 4 spaces indentation, and spaces after the commas.
  18. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues
    space before the +
  19. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues
    Another linebreak before this function definition
  20. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues
    Space before the +
  21. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues
    Another linebreak before this function definition.
  22. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues
    No linebreak required here.
  23. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues
    A space before the +
  24. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues
    Another linebreak before this function definition.
  25. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues
    No linebreak required here.
  26. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues
    Another linebreak before this function definition.
  27. reviewboard/admin/widgets.py (Diff revision 5)
     
     
     
    Show all issues
    Space after the commas.
  28. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues
    Another linebreak before this function definition.  Same applies to the rest of this review request, so I won't mention it anymore.
  29. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues
    Space before + sign
  30. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues
    Should this be a constant declared somewhere?
  31. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues
    no linebreak here.
  32. reviewboard/admin/widgets.py (Diff revision 5)
     
     
     
    Show all issues
    Only need a single linebreak here
  33. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues
    No linebreak here
  34. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues
    Space after #
  35. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues
    Is this line <= 80 chars?
  36. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues
    Only one space before "in", and there should be a linebreak before this for block.
  37. reviewboard/admin/widgets.py (Diff revision 5)
     
     
     
     
     
    Show all issues
    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.
  38. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Space after #
  39. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues
    Only one space before in, and a linebreak before the for block.  Same for below.
  40. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues
    Space after #
  41. reviewboard/admin/widgets.py (Diff revision 5)
     
     
     
     
     
     
     
    Show all issues
    Spaces after commas.
  42. reviewboard/htdocs/media/rb/css/admin.css (Diff revision 5)
     
     
     
     
     
    Show all issues
    The CSS params should be in alphabetical order, so:
    
    display: inline;
    padding: 0;
    position: relative;
    vertical-align: middle;
    
    Same applies below.
  43. Show all issues
    Two space indentation here.
  44. reviewboard/htdocs/media/rb/css/admin.css (Diff revision 5)
     
     
     
     
     
     
     
     
    Show all issues
    Two space indentation.
  45. Show all issues
    Two space indentation
  46. reviewboard/htdocs/media/rb/css/admin.css (Diff revision 5)
     
     
     
    Show all issues
    Two space indentation.  Other instances below.
  47. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 5)
     
     
     
     
     
     
     
     
    Show all issues
    Four space indentation.
  48. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 5)
     
     
    Javascript variables should always be declared with "var", so:
    
    var centerWidth...
    var docWidth...
  49. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 5)
     
     
    Show all issues
    Space after ,
  50. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 5)
     
     
    Show all issues
    var before widgetBoxId
  51. reviewboard/templates/admin/admin_widget.html (Diff revision 5)
     
     
     
     
    Show all issues
    Maybe we can move the {% if widget_icon %} just above <img src...>, and the {% endif %} right below, just to make things a little clearer.
  52. Show all issues
    No space before this <link> tag.
  53. Show all issues
    Needs to be in alphabetical order.
  54. Show all issues
    No space just before this <script> tag.
  55. Show all issues
    No linebreak required here.
  56. reviewboard/templates/admin/widgets/w-actions.html (Diff revision 5)
     
     
     
     
     
    Show all issues
    Alphabetical order, please
  57. Show all issues
    Proper indentation for the <h1> and <div> tags please.  More instances below.
  58. Show all issues
    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 %}
  59. reviewboard/templates/admin/widgets/w-groups.html (Diff revision 5)
     
     
     
     
     
     
     
    Show all issues
    Is that four spaces for indenting?  Not sure, but it seems like it's a bit more.
  60. Show all issues
    This line could probably be made clearer:
    
    <p class="no-result">
      {% trans... %}
      <br />
      <a href...
    </p>
  61. reviewboard/templates/admin/widgets/w-news.html (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Four spaces for indentation.
  62. Show all issues
    No spaces on either side of 'reload'
  63. Show all issues
    No linebreak between these.
  64. Show all issues
    Can this be split up into more than 1 line for clarity?
  65. Show all issues
    No space before the {%
  66. Show all issues
    I think this block of HTML can be un-indented a few spaces...
  67. Show all issues
    Two spaces, not one before these tags.
  68. reviewboard/templates/admin/widgets/w-stats-large.html (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Needs to be properly indented.
  69. Show all issues
    No space before the {
  70. Show all issues
    Should be "var y..."
  71. Show all issues
    Should be "var input_range_end", and the code in this function needs to be properly indented.
    1. the code goes:
      "  var input_range_start = $("#data-activity-start").val(),
                input_range_end = $("#data-activity-end").val();
      "
      as in http://www.jslint.com/lint.html#global
       
      But I can change it and make it into 2 vars if you want
  72. Show all issues
    "Comments", "Reviews", etc, should be in translate functions.
  73. 
      
VL
VL
VL
chipx86
  1. 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.
  2. reviewboard/admin/signals.py (Diff revision 8)
     
     
     
     
    Show all issues
    Imports must be alphabetical.
  3. reviewboard/admin/signals.py (Diff revision 8)
     
     
     
     
     
    Show all issues
    This must be inside the function, in the form of:
    
    def foo():
        """One-line summary
    
        Further multi-line description.
        """
        code
  4. reviewboard/admin/signals.py (Diff revision 8)
     
     
    Show all issues
    delete_widget_cache. We don't use CamelCase for functions.
  5. reviewboard/admin/signals.py (Diff revision 8)
     
     
     
    Show all issues
    This should probably be a constant defined outside the function. In this case, you'd call it CACHED_WIDGETS.
  6. reviewboard/admin/signals.py (Diff revision 8)
     
     
    Show all issues
    Space between parameters, after the comma.
  7. reviewboard/admin/signals.py (Diff revision 8)
     
     
    Show all issues
    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.
  8. Show all issues
    request is already defined above.
  9. Show all issues
    Blank line between these.
  10. Show all issues
    request is already defined above.
  11. Show all issues
    if ('_popup' not in request.REQUEST or
        'pop' not in request.REQUEST):
  12. reviewboard/admin/templatetags/rbadmintags.py (Diff revision 8)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    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.
  13. Show all issues
    request is already defined above.
  14. Show all issues
    This is sort of weird. Maybe pull this out above and pass in as a var.
  15. Show all issues
    request is used above.
  16. reviewboard/admin/views.py (Diff revision 8)
     
     
    Show all issues
    Alphabetical order.
    
    Can you go through the rest of your change and make sure imports are in alphabetical order?
  17. reviewboard/admin/views.py (Diff revision 8)
     
     
     
     
    Show all issues
    Two blank lines.
  18. reviewboard/admin/views.py (Diff revision 8)
     
     
    Show all issues
    So I'm not sure we should have this here. Instead, these should be webapi resources.
  19. reviewboard/admin/views.py (Diff revision 8)
     
     
     
     
     
    Show all issues
    Follow the doc format outlined above.
    
    Same with all other instances in your change.
  20. reviewboard/admin/views.py (Diff revision 8)
     
     
     
     
    Show all issues
    I'd suggest:
    
    state = request.GET.get('collapse', None)
    widget = ...
    
    if state and widget:
  21. reviewboard/admin/views.py (Diff revision 8)
     
     
    Show all issues
    widget_sets.
    
    Can you go through your diff and make sure all variables and functions are in this_style instead?
  22. reviewboard/admin/views.py (Diff revision 8)
     
     
     
     
    Show all issues
    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.
  23. reviewboard/admin/widgets.py (Diff revision 8)
     
     
    Show all issues
    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.
  24. reviewboard/admin/widgets.py (Diff revision 8)
     
     
    Show all issues
    No need for the \ on these. It's covered by being in the {}
  25. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8)
     
     
     
     
     
     
     
     
     
     
    Show all issues
    4 space indentation.
  26. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8)
     
     
    Show all issues
    Should tack onto the same var list, instead of adding a new "var" statement.
    
    Maybe just "width"?
  27. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8)
     
     
     
    Show all issues
    This can be:
    
    $(".admin-extras")
        .width(widSpace)
        .masonry("reload");
  28. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8)
     
     
    Show all issues
    Can you explicitly do $(document).ready(..) ?
  29. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8)
     
     
    Show all issues
     space indentation.
  30. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8)
     
     
    Show all issues
    No spaces inside the ()
  31. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8)
     
     
    Show all issues
    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.
  32. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8)
     
     
     
     
    Show all issues
    Indentation is off.
    
    No need for that empty function.
  33. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8)
     
     
    Show all issues
    There's an extra space at the beginning.
  34. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8)
     
     
    Show all issues
    Space after "if"
  35. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8)
     
     
     
     
    Show all issues
    No blank line.
  36. Show all issues
    Group this with the CSS above.
  37. reviewboard/templates/admin/settings.html (Diff revision 8)
     
     
     
     
    Show all issues
    Indentation is inconsistent.
    
    Make sure your HTML in this change is using 1 space indentation.
  38. Show all issues
    You shouldn't embed styles. Instead, use a class and define the rule.
  39. Show all issues
    Same here about the style.
  40. Show all issues
    $(document).ready()
  41. Show all issues
    No space before xaxis.
  42. Show all issues
    Only one blank line needed.
    
    Indentation is inconsistent.
  43. reviewboard/templates/admin/widgets/w-stats-large.html (Diff revision 8)
     
     
     
     
     
     
     
     
     
     
     
    Should be:
    
    $("<div id='tooltip'/>")
        .html(contents)
        .css({
            ...
        })
        .appendTo("body")
        .fadeIn(200);
  44. Show all issues
    Really should define #tooltip once so we don't have to fetch it each time.
  45. Show all issues
    No blank line.
  46. reviewboard/templates/admin/widgets/w-stats-large.html (Diff revision 8)
     
     
     
     
     
     
     
    Show all issues
    Indentation is wrong.
  47. < 80 chars.
  48. Show all issues
    Shouldn't embed style. Instead, define a CSS rule.
  49. 
      
chipx86
  1. Fixed up the style issues and pushed to master.
  2. 
      
VL
Review request changed
Status:
Completed
Change Summary:
Pushed to master (6b62945)