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

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

vladikoff
Review Board
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.

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)
     
     
     
     
     
     
    These changes should be pulled out into a separate review request.
  3. reviewboard/admin/templatetags/rbadmintags.py (Diff revision 1)
     
     
     
     
     
    django and djblets are both 3rd-party modules and should be grouped together.
  4. reviewboard/admin/templatetags/rbadmintags.py (Diff revision 1)
     
     
     
     
    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)
     
     
     
     
    There's some alignment issues here. It looks like several of these are indented only 3 spaces instead of 4.
  7. Same here re: MEDIA_URL and MEDIA_SERIAL
  8. reviewboard/admin/views.py (Diff revision 1)
     
     
    Remove this line.
  9. reviewboard/admin/views.py (Diff revision 1)
     
     
    Remove this line.
  10. reviewboard/admin/widgets.py (Diff revision 1)
     
     
     
     
    These should be prefixed with the reviewboard. module.
  11. reviewboard/admin/widgets.py (Diff revision 1)
     
     
     
    This should be a docstring.
  12. 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].
  13. reviewboard/admin/widgets.py (Diff revision 1)
     
     
    Please include "TODO" in this comment.
  14. 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,
    }
    
    ...
  15. reviewboard/admin/widgets.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    You can initialize this all at once.
  16. 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.
  17. 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?
  18. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 1)
     
     
    Space between // and the comment text. Same below.
  19. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    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. 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)
     
     
     
     
     
     
    These should all have ?{{MEDIA_SERIAL}} at the end.
  23. Please change this to be "Review Request Status"
  24. reviewboard/templates/admin/feed.html (Diff revision 1)
     
     
     
     
    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)
     
     
     
     
    These need to be placed in alphabetical ordering.  django.contrib... should be before django.core...
  3. Should be in alphabetical ordering - ie, at the top of the from imports.
  4. 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. I believe this needs to be indented four spaces.
  6. reviewboard/admin/views.py (Diff revision 5)
     
     
    I believe this should go with the group of "from" imports below this block.
  7. reviewboard/admin/views.py (Diff revision 5)
     
     
    one space after =
  8. reviewboard/admin/views.py (Diff revision 5)
     
     
    No linebreak here
  9. reviewboard/admin/views.py (Diff revision 5)
     
     
    A linebreak before this if block
  10. reviewboard/admin/views.py (Diff revision 5)
     
     
     
    This might look better with the line break after "(activity_data),", as opposed to after "HttpResponse("
  11. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Like above, I think this from import should join the group of from imports below.
  12. 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
  13. reviewboard/admin/widgets.py (Diff revision 5)
     
     
     
    Need to be put in alphabetical ordering.
  14. 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.
  15. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Another linebreak before the start of this function.
  16. 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"?
  17. reviewboard/admin/widgets.py (Diff revision 5)
     
     
     
    Should be 4 spaces indentation, and spaces after the commas.
  18. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    space before the +
  19. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Another linebreak before this function definition
  20. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Space before the +
  21. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Another linebreak before this function definition.
  22. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    No linebreak required here.
  23. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    A space before the +
  24. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Another linebreak before this function definition.
  25. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    No linebreak required here.
  26. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Another linebreak before this function definition.
  27. reviewboard/admin/widgets.py (Diff revision 5)
     
     
     
    Space after the commas.
  28. 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.
  29. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Space before + sign
  30. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Should this be a constant declared somewhere?
  31. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    no linebreak here.
  32. reviewboard/admin/widgets.py (Diff revision 5)
     
     
     
    Only need a single linebreak here
  33. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    No linebreak here
  34. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Space after #
  35. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Is this line <= 80 chars?
  36. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Only one space before "in", and there should be a linebreak before this for block.
  37. 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.
  38. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Space after #
  39. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Only one space before in, and a linebreak before the for block.  Same for below.
  40. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Space after #
  41. reviewboard/admin/widgets.py (Diff revision 5)
     
     
     
     
     
     
     
    Spaces after commas.
  42. 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.
  43. Two space indentation here.
  44. reviewboard/htdocs/media/rb/css/admin.css (Diff revision 5)
     
     
     
     
     
     
     
     
    Two space indentation.
  45. Two space indentation
  46. reviewboard/htdocs/media/rb/css/admin.css (Diff revision 5)
     
     
     
    Two space indentation.  Other instances below.
  47. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 5)
     
     
     
     
     
     
     
     
    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)
     
     
    Space after ,
  50. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 5)
     
     
    var before widgetBoxId
  51. 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.
  52. No space before this <link> tag.
  53. Needs to be in alphabetical order.
  54. No space just before this <script> tag.
  55. No linebreak required here.
  56. reviewboard/templates/admin/widgets/w-actions.html (Diff revision 5)
     
     
     
     
     
    Alphabetical order, please
  57. Proper indentation for the <h1> and <div> tags please.  More instances below.
  58. 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)
     
     
     
     
     
     
     
    Is that four spaces for indenting?  Not sure, but it seems like it's a bit more.
  60. 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)
     
     
     
     
     
     
     
     
     
     
     
    Four spaces for indentation.
  62. No spaces on either side of 'reload'
  63. No linebreak between these.
  64. Can this be split up into more than 1 line for clarity?
  65. No space before the {%
  66. I think this block of HTML can be un-indented a few spaces...
  67. Two spaces, not one before these tags.
  68. reviewboard/templates/admin/widgets/w-stats-large.html (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
    Needs to be properly indented.
  69. No space before the {
  70. Should be "var y..."
  71. 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. "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)
     
     
     
     
    Imports must be alphabetical.
  3. 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
  4. reviewboard/admin/signals.py (Diff revision 8)
     
     
    delete_widget_cache. We don't use CamelCase for functions.
  5. 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.
  6. reviewboard/admin/signals.py (Diff revision 8)
     
     
    Space between parameters, after the comma.
  7. 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.
  8. request is already defined above.
  9. Blank line between these.
  10. request is already defined above.
  11. if ('_popup' not in request.REQUEST or
        'pop' not in request.REQUEST):
  12. 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.
  13. request is already defined above.
  14. This is sort of weird. Maybe pull this out above and pass in as a var.
  15. request is used above.
  16. 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?
  17. reviewboard/admin/views.py (Diff revision 8)
     
     
     
     
    Two blank lines.
  18. reviewboard/admin/views.py (Diff revision 8)
     
     
    So I'm not sure we should have this here. Instead, these should be webapi resources.
  19. reviewboard/admin/views.py (Diff revision 8)
     
     
     
     
     
    Follow the doc format outlined above.
    
    Same with all other instances in your change.
  20. reviewboard/admin/views.py (Diff revision 8)
     
     
     
     
    I'd suggest:
    
    state = request.GET.get('collapse', None)
    widget = ...
    
    if state and widget:
  21. 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?
  22. 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.
  23. 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.
  24. reviewboard/admin/widgets.py (Diff revision 8)
     
     
    No need for the \ on these. It's covered by being in the {}
  25. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8)
     
     
     
     
     
     
     
     
     
     
    4 space indentation.
  26. 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"?
  27. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8)
     
     
     
    This can be:
    
    $(".admin-extras")
        .width(widSpace)
        .masonry("reload");
  28. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8)
     
     
    Can you explicitly do $(document).ready(..) ?
  29. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8)
     
     
     space indentation.
  30. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8)
     
     
    No spaces inside the ()
  31. 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.
  32. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8)
     
     
     
     
    Indentation is off.
    
    No need for that empty function.
  33. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8)
     
     
    There's an extra space at the beginning.
  34. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8)
     
     
    Space after "if"
  35. reviewboard/htdocs/media/rb/js/admin.js (Diff revision 8)
     
     
     
     
    No blank line.
  36. Group this with the CSS above.
  37. reviewboard/templates/admin/settings.html (Diff revision 8)
     
     
     
     
    Indentation is inconsistent.
    
    Make sure your HTML in this change is using 1 space indentation.
  38. You shouldn't embed styles. Instead, use a class and define the rule.
  39. Same here about the style.
  40. $(document).ready()
  41. No space before xaxis.
  42. 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. Really should define #tooltip once so we don't have to fetch it each time.
  45. No blank line.
  46. reviewboard/templates/admin/widgets/w-stats-large.html (Diff revision 8)
     
     
     
     
     
     
     
    Indentation is wrong.
  47. < 80 chars.
  48. 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: Closed (submitted)

Change Summary:

Pushed to master (6b62945)
Loading...