Adding support for alphabetic pagination to the datagrid.
Review Request #6369 — Created Sept. 24, 2014 and submitted
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_conley
|
|
|
Col: 80 E501 line too long (83 > 79 characters) |
 reviewbot
|
|
|
In our python code, dictionary entries should always have trailing commas |
 david
|
|
|
This suddenly made this quite complicated. Maybe we could have separate templates for the numeric paginator and the alphabetic one? |
david
|
|
|
'string' is a standard library module, so it should go in its own section: from __future__ import unicode_literals import string … |
david
|
|
|
Can you use single quotes around the 0? |
david
|
|
|
Col: 54 E711 comparison to None should be 'if cond is not None:' |
reviewbot
|
|
|
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
Col: 80 E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
Since this is the same between the two, can you pull it out into a helper method? |
david
|
|
|
Col: 1 W391 blank line at end of file |
reviewbot
|
|
|
Col: 30 W292 no newline at end of file |
reviewbot
|
|
|
Can you rewrap this? @register.inclusion_tag('datagrid/alphabetic_paginator.html', takes_context=True) def alphabetic_paginator(context, adjacent_pages=3): |
david
|
RM
- Change Summary:
-
Updated the template to that the paginator links provide consistent and correct URL arguments.
Fixed an issue with the ?letter argument being displayed twice because it was considered an extra query - Description:
-
~ 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).
~ As mentioned in 6369: ~ - It might make more sense for the filtering logic to occur on this side (as opposed to within the calling object, such as user_list). ~ 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.
- - I provided a screenshot of the current look of the new paginator, and and open to suggestions for CSS changes. - - I am curious if there is a more clever approach to templating that doesn't have so much repeated code. - Testing Done:
-
~ TODO
~ ~ The paginator currently does not work with usernames that do not start with A-Z.
~ TODO:
+ The paginator currently does not work cover usernames that do not start with A-Z. + Unit testing + + Done:
+ 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). - Commit:
-
78629a25a917d87572c4394aabffff01d6498b591d5901911de592b7320c24b4d7b250f531507d9c
-
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 -
RM
- Change Summary:
-
Fixed style issue with line length
- Commit:
-
1d5901911de592b7320c24b4d7b250f531507d9c65b6c726c2fe51e0390bfd411be62819bcb66a47
-
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
AD
-
-
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 ...
RM
- Change Summary:
-
Added a 0 button to select non-alphabetic names on the alphabetic paginator.
The paginator template file is now always called, but if there is 0 or 1 pages, than it will not display anything. - Commit:
-
65b6c726c2fe51e0390bfd411be62819bcb66a4773e7a69bf1a1631524c09a27f9dab29934808bff
RM
- Testing Done:
-
- - TODO:
- The paginator currently does not work cover usernames that do not start with A-Z. Unit testing Done:
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).
RM
- Change Summary:
-
Simplified the template changes from the last commit. Now the alphabetic paginator will always show if current_letter is provided, and will not display page number info if there is only 1 page.
Added brackets to surround the number info as suggested by Andrew. - Commit:
-
73e7a69bf1a1631524c09a27f9dab29934808bff561c7633eff16f4a8e8e4619ffd8f1a8e7ccf556
- Diff:
-
Revision 5 (+49 -14)
- Added Files:
RM
- Change Summary:
-
Changed from WIP to ready to review. Passed all unit tests.
- Summary:
-
[WIP] Adding support for alphabetic pagination to the datagrid.Adding support for alphabetic pagination to the datagrid.
- Description:
-
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.
- Testing Done:
-
~ TODO:
~ Unit testing ~ ~ 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). - Done:
- 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).
RM
- Change Summary:
-
Addressed some style issues.
Broke up the paginator into two mutually exclusive templates (alphabetic and numeric) which are selected by the listview. - Commit:
-
561c7633eff16f4a8e8e4619ffd8f1a8e7ccf556f52555a433130502769df10b4ca96d6309535221
- Diff:
-
Revision 6 (+94 -21)
-
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 -
-
-
RM
- Change Summary:
-
Corrected some reviewbot style issues.
- Commit:
-
f52555a433130502769df10b4ca96d6309535221825568d0c95c56227623996a2be6464ed11fbc02
- Diff:
-
Revision 7 (+96 -21)
-
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
RM
- Change Summary:
-
Cleaned up some redundant code use in the paginator template context.
- Commit:
-
825568d0c95c56227623996a2be6464ed11fbc024680c0172baa3a55f78b55b9a06f8dbf8c93e3ad
- Diff:
-
Revision 8 (+84 -21)
-
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 -
RM
- Change Summary:
-
Removing empty line style issue
- Commit:
-
4680c0172baa3a55f78b55b9a06f8dbf8c93e3ad8d6ce79128839607e15ad5a71dd75ce6eacc3896
- Diff:
-
Revision 9 (+83 -21)
-
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 -
RM
- Change Summary:
-
Resolved missing newline warning
- Commit:
-
8d6ce79128839607e15ad5a71dd75ce6eacc3896c93c1008d19c7554287a2da2c750808ba3344e02
- Diff:
-
Revision 10 (+83 -21)
-
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
-
-
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?
RM
- Change Summary:
-
As per m_conleys suggestion, wrapped the selected letter controls in a seperate span class and changed the background color CSS.
Also removed some trailing </span> tags from the paginator templates that weren't doing anything. - Commit:
-
c93c1008d19c7554287a2da2c750808ba3344e026347e4ad084c0a51c024f51585d28f31e1af4bc9
- Diff:
-
Revision 11 (+96 -22)
- Added Files:
-
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
-
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?
RM
- Change Summary:
-
Removed borders and brackets from the numeric controls (for the selected letter). Set numeric controls background to light blue with border. (See screenshot paginator-final.png)
- Commit:
-
6347e4ad084c0a51c024f51585d28f31e1af4bc998e0fb7f02d170e8efdb7d81a1b71ecbb986de7d
- Diff:
-
Revision 12 (+110 -21)
- Added Files:
-
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
RM
- Change Summary:
-
Correcting line wrap issue in templatetags/datagrid as suggested by David.
- Commit:
-
98e0fb7f02d170e8efdb7d81a1b71ecbb986de7d6beefef9410d7ea0427dd0c8d281f2a322c2658d
- Diff:
-
Revision 13 (+110 -21)
-
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
-
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) Theif pages != 0 and pages != 1should probably just bepages > 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.
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