Improved usability by adding a tabbish feel to admin navbar.

Review Request #1029 — Created Sept. 8, 2009 and submitted

Information

Review Board

Reviewers

When configuring ReviewBoard is really easy to loose track on which tab you are, since there was no visual cue to that.
There was a css class already in admin.css called '#admin-subnav li a.active', but it was never used.

So I hooked it up in the code, so it stared being used. But then it clashed too much, and the tabs didn't feel like tabs. So I changed it to include a background and a border so users can instantly recognize where they are.

Since the breadcrumbs part has a little gradient, I had to expand that gradient otherwise the background clashed.
Tested result in Safari 4.0, Firefox 3.5 and 3.0 and Chromium Nightly.
chipx86
  1. Hey Eduardo,
    
    Thanks for the change. I like the new look a lot better.
    
    I have some ideas on the implementation, though. I think rather than capturing the page name in the URLs, we should keep all logic about the current page in the view instead. I think we can actually 
    
    The way we can do this is to implement a new templatetag for the admin UI for adding a nav item. We'd use it like:
    
    {% admin_subnav "url-name" _("Text") %}
    
    The implementation for this would add the <li>...</li>, resolving url-name using reverse() (something I've wanted to do for a while, instead of hard-coding the paths -- this means we need to name those URLs too), and applying the "active" class if needed.
    
    The template tag would determine the active URL by comparing the result of reverse(url_name) with request.path.
    
    The nice thing here is that all logic for this goes right into the template tag, and we have a nice reusable thing in the templates that doesn't have to care about the active state itself.
    
    How does that sound?
    
    Also, ping me on IRC at some point. I seem to have issues e-mailing your account.
  2. Should be indented 2 spaces.
  3. We should make sure that -6px is correct for this case. If you increase your text zoom a lot, does it look okay? We may want to use em.
  4. Should just be indented 2 spaces.
  5. 
      
edufelipe
  1. 
      
  2. Oooops. I had forgotten the shortcut for it, heheh.
  3. 
      
edufelipe
edufelipe
chipx86
  1. 
      
  2. reviewboard/admin/templatetags/admintags.py (Diff revision 3)
     
     
     
    Blank line between these.
    1. I don't get this guideline well. On other reviews you insisted on joining django and djblets on the import part. Now I should split them. Is there an heuristic to figure it out?
      
      Either way it's no longer relevant as I'm not using basictag anymore.
    2. You're completely right. I was not thinking when I wrote this.
    3. Christian was wrong--I think he just got confused for a moment. Both django and djblets are "3rd-party" modules, which mean they go together in the second block of imports.
  3. reviewboard/admin/templatetags/admintags.py (Diff revision 3)
     
     
     
     
     
    Actually, it might be better to render using a template which would have this logic. While technically slower, it keeps presentation in the templates.
    
    You can do this with an inclusion tag, which the docs should cover.
  4. reviewboard/admin/templatetags/admintags.py (Diff revision 3)
     
     
     
    Can remove these blank lines.
  5. reviewboard/admin/urls.py (Diff revision 3)
     
     
    Can you specifically do name="general", etc. on these?
    1. In order to do this I would have to change them all to use the url() construct, instead of doing it inline. But if you prefer i could.
    2. The url() construct is perfectly fine. We do this everywhere else in our URLs where we name them.
  6. Hmm, maybe we should call these rbadmintags, so that there's no possible conflict with Django's own admin stuff.
  7. reviewboard/templates/admin/settings.html (Diff revision 3)
     
     
     
     
     
    You should use _("General"), etc. so that it's internationalized.
    
    You'll need to {% load i18n %} for this, I think.
  8. 
      
edufelipe
chipx86
  1. Hi Eduardo.
    
    Just a few small stylistic things and then it's ready to commit :) We need to set you up on GitHub with commit access, so let us know what your GitHub username is (if you have one) and we'll set it up.
  2. This would be better formatted as:
    
    return {
        'url': url,
        'name': name,
        'current': url == request.path,
    }
  3. reviewboard/admin/urls.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
    Make sure the {...} aligns with the r'...'.
    
    Also, name="logging" would be better on its own line, aligned with r'..' and {..}
  4. reviewboard/templates/admin/settings.html (Diff revision 4)
     
     
     
    Swap these. Alphabetical order.
  5. Variables (as opposed to template tags) should be in the form of {{url}}.
  6. 
      
edufelipe
Review request changed
chipx86
  1. 
      
  2. reviewboard/admin/urls.py (Diff revision 5)
     
     
    Really sorry, but I didn't think of this before. Can you change the names to settings-general, settings-email, etc? Less chance of collisions with other things. Make sure to update the template as well.
    
    Then it's ready to commit! :) You'll need to edit your .git/config and change origin's url to git@github.com:reviewboard/reviewboard.git. Then squash this into one commit in master preferably so it doesn't appear as a merge, add the "Reviewed at http://reviews.review-board.org/r/1029/" and then `git push origin master`.
  3.