Adding support for alphabetic pagination to the datagrid.

Review Request #6369 — Created Sept. 24, 2014 and submitted

Information

Djblets
master
829
6beefef...

Reviewers

Updating the paginator in datagrid to allow alphabetic querying to be turned on by providing the DataGrid object with the current_letter parameter (default none).

If the current_letter is provided to the datagrid (usually by some child grid calling the super initializer) the datagrid will simply load a second template case added paginator.html.

It is the job of the child class (UserGrid in reviewboard) to process the URI, perform filtering on the queryset, and handle errors.

Unit tests all pass
All button cases manually tested. (Forward, back, first and last arrows, each letter).
Verified that changes to not affect any other module that inherits from datagrid (such as the groups list, or the all review requests list, the current 'master' of reviewboard with no changes to the userGrid).


Description From Last Updated

So I was thinking about this some - it's really difficult (even with the brackets in the other screenshot) to …

mike_conleymike_conley

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

In our python code, dictionary entries should always have trailing commas

daviddavid

This suddenly made this quite complicated. Maybe we could have separate templates for the numeric paginator and the alphabetic one?

daviddavid

'string' is a standard library module, so it should go in its own section: from __future__ import unicode_literals import string …

daviddavid

Can you use single quotes around the 0?

daviddavid

Col: 54 E711 comparison to None should be 'if cond is not None:'

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Since this is the same between the two, can you pull it out into a helper method?

daviddavid

Col: 1 W391 blank line at end of file

reviewbotreviewbot

Col: 30 W292 no newline at end of file

reviewbotreviewbot

Can you rewrap this? @register.inclusion_tag('datagrid/alphabetic_paginator.html', takes_context=True) def alphabetic_paginator(context, adjacent_pages=3):

daviddavid
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/paginator.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/paginator.html
    
    
  2. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/paginator.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/paginator.html
    
    
  2. djblets/datagrid/grids.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/paginator.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/paginator.html
    
    
  2. 
      
RM
AD
  1. 
      
  2. Loving this functionality, looks great so far!
    I know this is a bit early but do you think brackets around the number paginator and the page count would make it look better? I was a little confused when I first saw this so just a suggestion.

    Something like

    ... C D ( << < 1 2 3 4 5 > >> 6 pages ) E F G ...

    1. I tried this out in my latest changes. There is a new screen cap to see how it looks.

    2. Nice, it seems to work, in my opinion :)

  3. 
      
RM
RM
RM
RM
david
  1. 
      
  2. djblets/datagrid/grids.py (Diff revision 5)
     
     

    In our python code, dictionary entries should always have trailing commas

  3. This suddenly made this quite complicated. Maybe we could have separate templates for the numeric paginator and the alphabetic one?

  4. djblets/datagrid/templatetags/datagrid.py (Diff revision 5)
     
     
     
     
     

    'string' is a standard library module, so it should go in its own section:

    from __future__ import unicode_literals
    
    import string
    
    from django import template
    
  5. Can you use single quotes around the 0?

  6. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
  2. djblets/datagrid/grids.py (Diff revision 6)
     
     
    Col: 54
     E711 comparison to None should be 'if cond is not None:'
    
  3. Col: 1
     E302 expected 2 blank lines, found 1
    
  4. Col: 80
     E501 line too long (81 > 79 characters)
    
  5. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
  2. 
      
david
  1. 
      
  2. djblets/datagrid/templatetags/datagrid.py (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Since this is the same between the two, can you pull it out into a helper method?

  3. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
  2. Col: 1
     W391 blank line at end of file
    
  3. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
  2. Col: 30
     W292 no newline at end of file
    
  3. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
  2. 
      
mike_conley
  1. 
      
  2. So I was thinking about this some - it's really difficult (even with the brackets in the other screenshot) to distinguish between both:

    a) The selected letter to the rest of the symbols

    b) The page numeric page selectors from the rest of the symbols

    So what I suggest is toying around with putting a more eye-catching background colour around the selected letter (and maybe a more eye-catching foreground colour on the letter itself), and then stretching those colours behind the numeric page selectors as well.

    Can you experiment with that and show us what that might look like?

  3. 
      
RM
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/static/djblets/css/datagrid.less
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/static/djblets/css/datagrid.less
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
  2. 
      
mike_conley
  1. Ok, glad to know it can be done - however, I was thinking of something a little less subtle - something more to draw the eye.

    Notice the slight vertical gradient behind the unselected letters? That gradient is similar to the ones for the headers in the datagrid of the Dashboard, and those headers have a hover effect that draws the eye:

    https://www.dropbox.com/s/f0j5t6ybndbgga9/Screenshot%202014-10-19%2019.55.19.png?dl=0

    Could you try something like that?

    1. https://dl.dropboxusercontent.com/u/6837632/paginator-gradient.png

      This borrows the gradient from the datagrid header. I also set up the mouseover functionality to change to a more active color when the mouse is over that area, which would be useful even if we went back to solid color.

      Since these are really minute CSS changes I'll just preview them here for now?

    2. Ok, thanks for trying that. I think I like the blue colour, but not the gradient so much.

      Can we keep the stylings in the bottom half of the screenshot (hovering or not), but get rid of the white stripe along the top? I think perhaps that blue might be just enough to draw the eye.

    3. https://dl.dropboxusercontent.com/u/6837632/paginator-blue.png

      I added a grey border around the area too and I think it holds it together a lot better. Blue always on? (So no mouseover needed)

    4. Yes, I think that is better (and yes, I think it should be without mouseover needed)...

      My last critique is the grey borders between the numbers / arrows. They're a bit hard to see over the blue, and yet they're minutely detectible, so the eyes strain to see what they are.

      I'm not entirely sure they're necessary. Can you try removing those grey borders? Let's get rid of those, and the parenthesis as well - perhaps like this?:

      https://www.dropbox.com/s/igprs5unnd6hgw6/Screenshot%202014-10-19%2021.33.08.png?dl=0

      What are your thoughts on that?

    5. https://dl.dropboxusercontent.com/u/6837632/paginator-final.png

      A lot less busy now. I like it. The numbers still have boxed highlighting when numeric links are moused over.

    6. I like it! Let's roll with that. Would you mind attaching your screenshot to this review request?

    7. Done!

  2. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/static/djblets/css/datagrid.less
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/static/djblets/css/datagrid.less
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
  2. 
      
david
  1. I love the new look!

  2. djblets/datagrid/templatetags/datagrid.py (Diff revision 12)
     
     
     

    Can you rewrap this?

    @register.inclusion_tag('datagrid/alphabetic_paginator.html',
                            takes_context=True)
    def alphabetic_paginator(context, adjacent_pages=3):
    
  3. 
      
RM
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/static/djblets/css/datagrid.less
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/static/djblets/css/datagrid.less
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
  2. 
      
david
  1. Ship It!

  2. 
      
RM
chipx86
  1. I'm sorry I didn't get to this before, but I actually have some things I'd like to see changed before we can really ship this.

    The main thing (which I thought I commented on once -- maybe in IRC? -- but perhaps not), is that this functionality should live in a DataGrid subclass, rather than DataGrid itself. Not all datagrids are really geared toward alphabetic pagination. In fact, out of the box, this change doesn't actually enable alphabetic pagination, instead relying on the subclass to do the work anyway.

    What I'd like to see instead is an AlphabeticDataGrid that handles the work of filtering that query, basically doing what UserDataGrid is currently doing. It would be given the name of the field that it should filter by, and do the filtering based on the letter or number. The base DataGrid would then not need any changes, and UserDataGrid would simply have to subclass the new AlphabeticDataGrid and provide the field name to filter on.

    I'd also like to see some cleanup in the paginator templates. Django templates are very slow, so ideally, we'd reduce the amount of work that needs to be done. Primarily this involves:

    1) Collapsing conditionals (there's some ifequals followed by ifs that can be combined into a single statement).
    2) Reducing the amount of conditionals and variable access (all those {% if extra_query %}{{extra_query}}&{% endif %} blocks can be computed once at the very top using {% definevar %} and just accessed in each case.
    3) The if pages != 0 and pages != 1 should probably just be pages > 1, right?

    Last thing: We need to preserve compatibility between releases, and deprecate things carefully. This change breaks compatibility by renaming the paginator.html template and paginator template tags. Instead, those old names should be preserved. It's okay to just have them call the new ones, but flat-out removing them means that any consumer that expects those to exist will break.

    1. Is it alright if I do this in a new review request then?

  2. 
      
RM
RM
Review request changed

Status: Closed (submitted)

Loading...