Feedback for Dashboard widget logic

Review Request #2361 — Created May 17, 2011 and discarded

Information

Review Board

Reviewers

I would like to get some feedback for this dashboard logic and code style formatting.

The dashboard.html template would have this:
Example: {% admin_widget "user-activity" _("User Activity") %}
I followed the same pattern as {% admin_subnav .... %}

'admin/admin_widget.html' - is a wrap for all widgets

'w-user-activity.html' - is a template that includes the necessary HTML and JavaScript for "User Activity" and is 'embedded' as '{% include widget_content %}' in 'admin_widget.html'

'rbadmintags.py' -  admin_widget(...) has a list of all possible widgets in "widgets = {}".

 
Description From Last Updated

You're going to want to put these spaces back. We put two spaces between class-level items in code.

mike_conleymike_conley

I think we'll want to be consistent with our naming, and call these media_url and media_serial.

mike_conleymike_conley

Hm - I'm not sure if it makes the most sense to put the widgets in here. Maybe there should …

mike_conleymike_conley

We're going to want this i18n'able.

mike_conleymike_conley

You'll probably want to put in a more descriptive comment here. ;)

mike_conleymike_conley

Hm - why the extraneous spaces around widget_data.x within the {'s and }'s? Some of these lines seem a bit …

mike_conleymike_conley

Same, re: more descriptive comment.

mike_conleymike_conley

Spaces on either side of the +

mike_conleymike_conley

"Total Users" needs to be wrapped up for i18n.

mike_conleymike_conley

Because we're starting to load from a different package, I think we put a newline between from django.core.urlresolvers... and from …

mike_conleymike_conley

Two spaces here.

mike_conleymike_conley

I'd like some documentation describing what this widget actually does.

mike_conleymike_conley

I have a feeling that some of these lines are still over the 80 char limit - can you break …

mike_conleymike_conley

{{widget_size}} as opposed to {{ widget_size }} - same with widget_name.

mike_conleymike_conley

See above.

mike_conleymike_conley

Any way of making "Active", "7 days ago", etc, i18n-able?

mike_conleymike_conley

Whoops - should have been more clear. I meant that there *should* be spaces on either side of the +'s. …

mike_conleymike_conley

I'm not sure this block needs to be indented.

mike_conleymike_conley
mike_conley
  1. Hey Vlad:
    
    Good work here.  I've found a few issues, and I have a few questions - see below.
    
    Thanks,
    
    -Mike
  2. reviewboard/admin/templatetags/rbadmintags.py (Diff revision 1)
     
     
     
     
     
    Show all issues
    You're going to want to put these spaces back.  We put two spaces between class-level items in code.
  3. Show all issues
    I think we'll want to be consistent with our naming, and call these media_url and media_serial.
  4. Show all issues
    Hm - I'm not sure if it makes the most sense to put the widgets in here.  Maybe there should be a widgets.py within the admin subdirectory?
  5. Show all issues
    We're going to want this i18n'able.
  6. Show all issues
    You'll probably want to put in a more descriptive comment here. ;)
  7. reviewboard/templates/admin/widgets/w-user-activity.html (Diff revision 1)
     
     
     
     
     
     
    Show all issues
    Hm - why the extraneous spaces around widget_data.x within the {'s and }'s?  Some of these lines seem a bit long and unruly to me.
    
    Also, those strings need to be i18n'able.
    1. The spaces were fixed. However, I was advised to drop this issue for now, until proper i18n functions are available.
  8. Show all issues
    Same, re: more descriptive comment.
  9. Show all issues
    Spaces on either side of the +
  10. Show all issues
    "Total Users" needs to be wrapped up for i18n.
  11. 
      
VL
mike_conley
  1. Vlad:
    
    It's looking good!  A few more things caught my eye this iteration - see below.
    
    Thanks,
    
    -Mike
  2. Show all issues
    Because we're starting to load from a different package, I think we put a newline between from django.core.urlresolvers... and from reviewboard.admin...
  3. reviewboard/admin/widgets.py (Diff revision 2)
     
     
    Show all issues
    Two spaces here.
  4. reviewboard/admin/widgets.py (Diff revision 2)
     
     
    Show all issues
    I'd like some documentation describing what this widget actually does.
  5. reviewboard/admin/widgets.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    I have a feeling that some of these lines are still over the 80 char limit - can you break them up a bit to shorten them?
  6. Show all issues
    {{widget_size}} as opposed to {{ widget_size }} - same with widget_name.
  7. Show all issues
    See above.
  8. reviewboard/templates/admin/widgets/w-user-activity.html (Diff revision 2)
     
     
     
     
     
     
    Show all issues
    Any way of making "Active", "7 days ago", etc, i18n-able?
  9. Show all issues
    Whoops - should have been more clear.  I meant that there *should* be spaces on either side of the +'s. :)
  10. Show all issues
    I'm not sure this block needs to be indented.
  11. 
      
VL
Review request changed
Status:
Discarded