Review Board user page (with hover preview).

Review Request #1953 — Created Nov. 29, 2010 and submitted

Information

Review Board

Reviewers

Currently when a user visits /users/<user name>, all of the review requests of that user are shown.  This patch modifies this page slightly to include more information.  This is a first step, and I've tried to make it as simple as possible.  Further modification will probably be needed.

I have also added a hover preview.  When the cursor moves over top of a user name a small preview of that user will be shown (see screenshot).  This feature is only supported on the review request pages (not in the "submitters" page for example).  This could be added later, but was omitted due to the possibility of too many images appearing.
Manual testing.  The way the hover preview works might need to be tested outside of the review request page.  

Also only tested on chrome and firefox.
chipx86
  1. 
      
  2. The popup bubble is cute, but I think we can do without the titlebar. It doesn't really add anything (we already know it's Review Board) and it takes up space.
  3. Can you use a <label> and appropriate style so it's bold and black for the labels?
  4. 
      
chipx86
  1. A few things that stand out immediately.
    
    1) Make sure to only use spaces, not tabs, for indentation. Much of the new code is using tabs, and it stands out in the diff viewer.
    
    2) The diff wasn't generated correctly and doesn't apply to a few of the files. How was the diff generated? Make sure it's a branch off of an up-to-date 'master' and you're using post-review.
  2. 
      
KF
KF
KF
chipx86
  1. Can you upload a new screenshot with the new hover?
    
    Sorry for the late review on this. You'll have it today.
  2. 
      
KF
chipx86
  1. 
      
  2. reviewboard/htdocs/media/rb/css/common.css (Diff revision 5)
     
     
     
     
    Two blank lines between these.
  3. reviewboard/htdocs/media/rb/css/common.css (Diff revision 5)
     
     
     
    Should be combined:
    
    margin: 18px auto 0 auto;
  4. reviewboard/htdocs/media/rb/css/common.css (Diff revision 5)
     
     
     
     
    Seems this should be alongside the ".review .reviewer" rule in reviews.css.
  5. reviewboard/htdocs/media/rb/css/common.css (Diff revision 5)
     
     
     
     
    Two blank lines between these.
  6. reviewboard/htdocs/media/rb/js/common.js (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    All this logic appears to be duplicated. It should be consolidated. You should be able to do:
    
    $("#submitter, .reviewer a").hover(...)
  7. reviewboard/htdocs/media/rb/js/common.js (Diff revision 5)
     
     
     
     
    I think this can be:
    
    $(this).append(
        $("<div .../>")
        .load($(this).attr(...)));
  8. reviewboard/reviews/views.py (Diff revision 5)
     
     
    No space before the "("
  9. reviewboard/reviews/views.py (Diff revision 5)
     
     
     
    This should include the local_site checks, like the other views.
  10. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Space after the ":" in the dictionary.
  11. reviewboard/templates/accounts/user_infobox.html (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Should have one space indentation for each nested tag.
    
    Also, can you use "user-infobox" instead of "preview" for the IDs? "preview" is too generic and could potentially be used in other places.
  12. I don't think we need the <br/>.
  13. reviewboard/templates/accounts/user_infobox.html (Diff revision 5)
     
     
     
     
     
    Instead of <b>..</b> and the <br/>, do:
    
    <p><label>{% trans "Name:" %}</label> {{whatever}}</p>
    
    You may need to tweak the styles for the margins on the <p> tags in this case. Maybe for <label> too, depending on which color you get (I imagine it'll be brown).
  14. The rest of the templates specifically specify a format string. This should use:
    
    {{user.last_login|date:"F jS, Y, P"}}
    
    Same with date_joined.
  15. reviewboard/templates/accounts/user_infobox.html (Diff revision 5)
     
     
     
     
     
     
    No blank lines between these.
  16. I imagine we should be able to use datagrid.html as the base?
  17. Pretty sure this isn't the title you want.
  18. Shouldn't reload.
  19. reviewboard/templates/reviews/user_page.html (Diff revision 5)
     
     
     
     
     
     
     
    If we use datagrid.html, we should get these for free.
  20. reviewboard/templates/reviews/user_page.html (Diff revision 5)
     
     
     
     
     
    Indentation is off on most things. Should always be consistent and use one space.
  21. Each of these should use <label> instead of <div class=".."> and localize the text using {% trans %}
    
    Shouldn't use <b> for the values, either.
    
    Also, make sure to use the values recommended above for the infobox.
  22. Use {% trans %} to localize this.
  23. Space after {%
  24. reviewboard/urls.py (Diff revision 5)
     
     
    This needs to do the same local_site_name URL matching and logic as the other URLs, so that users can't grab info on users from other sites.
  25. 
      
KF
Review request changed
chipx86
  1. Committed to master (4dd548286edf1bee390e7775984890145ada8385) along with some modifications to the hover box view to make it play nice with the new LocalSite stuff and to show the requested user instead of always request.user (we both missed this the first time).
  2. 
      
Loading...