Summary

Added CheckboxColumn for datagrids

Review Request #3796 — Created Jan. 28, 2013 and discarded Dec. 19, 2013, 3:08 a.m.

Information

Owner

Repository

Djblets

Branch

master

Bugs

Depends On

Reviewers

Groups

People

Description

Added CheckboxColumn for datagrids

This commit adds a new CheckboxColumn class inheriting from Column.
When added to a DataGrid this column will contain checkboxes, with a
checkbox in the column header that can check/uncheck all the checkboxes.

The is_selectable & is_selected functions can be overridden to control
whether a checkbox is displayed in a row and whether that checkbox is
initially checked.

The checkboxes have a data-object-id attribute that should allow
javascript code to determine which rows have been checked and use that
accordingly.

This should allow mass submit/discard buttons to be easily added to the
dashboard in reviewboard.

Testing Done

- Integrated into revivewboard (code in this review: http://reviews.reviewboard.org/r/3799/ )
- Confirmed that header checkbox selects/deselects properly
- Confirmed that header behaves as expected (movable, displays correctly etc.)
- Confirmed that it's possible to write javascript to check which rows are selected and act on it.

Issues

Description	From	Last Updated
The other columns don't really follow this yet, but this should be one line, like: """A column that renders a …	chipx86	Jan. 27, 2013, 8:31 p.m.
No need for this pass.	chipx86	Jan. 27, 2013, 8:31 p.m.
You don't need the + when concatenating strings. Removing it will let you remove one layer of parens here.	chipx86	Jan. 27, 2013, 8:32 p.m.
Should probably escape the contents of the label, in case it includes something like: '/>..	chipx86	Jan. 28, 2013, 10:11 p.m.
Should follow this convention: """One-line summary Description. """	chipx86	Jan. 27, 2013, 8:36 p.m.
Same here.	chipx86	Jan. 27, 2013, 8:36 p.m.
Missing a trailing period.	chipx86	Jan. 27, 2013, 8:36 p.m.
Not sure this should be here?	chipx86	Jan. 27, 2013, 8:36 p.m.
Should be able to fit the function on the first line.	chipx86	Jan. 27, 2013, 8:37 p.m.
Must use var statements. Should be like: var checked = ..., col = ...;	chipx86	Jan. 27, 2013, 8:38 p.m.
This will be faster if you make the selector more fine-grained. Should include the datagrid itself, if possible, and limit …	chipx86	Feb. 7, 2013, 9:35 p.m.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    djblets/datagrid/grids.py
  Ignored Files:
    djblets/datagrid/templates/datagrid/column_header.html
    djblets/media/js/datagrid.js

I'm excited about this change and the Review Board one :) Some things to fix though.

djblets/datagrid/grids.py (Diff revision 1)

The issue has been resolved. Show all issues

The other columns don't really follow this yet, but this should be one line, like:

"""A column that renders a checkbox."""

Ideally, it should also go into some detail on how it's used. Basically, what you have in your review request description.

djblets/datagrid/grids.py (Diff revision 1)
The issue has been resolved. Show all issues
```
No need for this pass.
```

djblets/datagrid/grids.py (Diff revision 1)

The issue has been resolved. Show all issues

You don't need the + when concatenating strings. Removing it will let you remove one layer of parens here.

djblets/datagrid/grids.py (Diff revision 1)

The issue has been resolved. Show all issues

Should probably escape the contents of the label, in case it includes something like: '/><script>..</script>

djblets/datagrid/grids.py (Diff revision 1)

The issue has been resolved. Show all issues

Should follow this convention:

"""One-line summary

Description.
"""

djblets/datagrid/grids.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same here.
```
djblets/datagrid/grids.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Missing a trailing period.
```
djblets/datagrid/grids.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Not sure this should be here?
```
djblets/media/js/datagrid.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Should be able to fit the function on the first line.
```
djblets/media/js/datagrid.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Must use var statements. Should be like:

var checked = ...,
    col = ...;
```

djblets/media/js/datagrid.js (Diff revision 1)

The issue has been resolved. Show all issues

This will be faster if you make the selector more fine-grained. Should include the datagrid itself, if possible, and limit it to input elements (input[data-checkbox-col....])

GC

gcoupar Jan. 28, 2013, 10:20 p.m.

I've added .datagrid as a selector, is that be good enough?  Wasn't sure if there was a reasonable way to get the specific datagrid that this checkbox is contained in.   Suppose I could find the parent of the checkbox that was clicked and then find children of that if you'd rather?

david Feb. 5, 2013, 2:19 a.m.

I think grabbing the parent and then finding children of that is a great solution. Review Board of course has no pages with multiple datagrids on it, but other users of djblets might.

GC

Change Summary:

Fixed all the issues pointed out (except possibly a selector).
Added a new parameter checkbox_column_name to be used for the data-checkbox-col HTML attribute, as the label will be localised and isn't really suitable for this.

Diff:

Revision 2 (+65)

	djblets/datagrid/grids.py
	djblets/datagrid/templates/datagrid/column_header.html
	djblets/media/js/datagrid.js

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    djblets/datagrid/grids.py
  Ignored Files:
    djblets/datagrid/templates/datagrid/column_header.html
    djblets/media/js/datagrid.js

GC

Change Summary:

Fixed indentation in column_header.html

Diff:

Revision 3 (+65)

	djblets/datagrid/grids.py
	djblets/datagrid/templates/datagrid/column_header.html
	djblets/media/js/datagrid.js

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    djblets/datagrid/grids.py
  Ignored Files:
    djblets/datagrid/templates/datagrid/column_header.html
    djblets/media/js/datagrid.js

GC

Change Summary:

Updated click handler for checkbox column header to find children of parent datagrid.

Diff:

Revision 4 (+66)

	djblets/datagrid/grids.py
	djblets/datagrid/templates/datagrid/column_header.html
	djblets/media/js/datagrid.js

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    djblets/datagrid/grids.py
  Ignored Files:
    djblets/datagrid/templates/datagrid/column_header.html
    djblets/media/js/datagrid.js

I know it's been a long time coming, but I'm working on getting these checkbox datagrid changes ported over to the new codebase and in for RB 2.0. I'll be discarding these review requests and pointing to the new changes. Thanks!

GC

Status: Discarded

Change Summary:

Modified a bit and put up again at /r/5141/

Loading...