Added mass submit/discard to dashboard
Review Request #3799 — Created Feb. 5, 2013 and submitted
Added mass submit/discard to dashboard This commit adds a CheckboxColumn to the datagrid in the dashboard, as well as some links in the datagrid header to submit or discard all the selected reviews. When clicked, the links will send API requests to close each of the selected reviews and will then reload the page. This code depends on the CheckboxColumn being added to djblets, which I've submitted in another review: http://reviews.reviewboard.org/r/3796/
- Confirmed that submit/discard selected buttons closes the reviews then reloads the page - Confirmed that check/uncheck all works
| Description | From | Last Updated |
|---|---|---|
|
Can you make this another paragraph? That way there dialog isn't as wide, and easier to scan. |
 chipx86
|
|
|
Col: 80 E501 line too long (83 > 79 characters) |
 reviewbot
|
|
|
Should only have one import, and then import CheckboxColumn on a second line, using \ for line continuation. CheckboxColumn would … |
chipx86
|
|
|
Labels are meant to be localized. This one should be. However, that means you'll have a problem with looking it … |
chipx86
|
|
|
What does all this cover? |
chipx86
|
|
|
Comments should be proper sentences and end in a trailing period. (Yes, it's nitpicky, but we strive for nitpicky consistency … |
chipx86
|
|
|
Really, this is "registerDashboardActions". |
chipx86
|
|
|
Should use one var statement, and comma-separate them (one per line). This query should be more limited in scope. It … |
chipx86
|
|
|
Trailing whitespace. |
chipx86
|
|
|
blank line between these. |
chipx86
|
|
|
Same comment about vars. Also, blank line after vars.No need for the 'self' one though. |
chipx86
|
|
|
Blank line. |
chipx86
|
|
|
Can you fix these vars while you're at it? |
chipx86
|
|
|
One space indentation. I think it'd be best if these IDs were more specific so they can't conflict. Prefix with … |
chipx86
|
-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/datagrids.py Ignored Files: reviewboard/static/rb/css/dashboard.less reviewboard/static/rb/js/common.js reviewboard/static/rb/js/datastore.js reviewboard/templates/reviews/dashboard_listview.html
-
Can you provide screenshots of this? One showing it off, one with the menus opened. One more addition I think is important is to prompt before making the action happen. It should ask an "Are you sure" question along the lines of our existing ones for, say, deletion (when you're an admin).
-
reviewboard/reviews/datagrids.py (Diff revision 2) Should only have one import, and then import CheckboxColumn on a second line, using \ for line continuation. CheckboxColumn would align with Column. ReviewBot is going to yell at you about this, but ignore it. It's one of those things we can't cleanly turn off...
-
reviewboard/reviews/datagrids.py (Diff revision 2) Labels are meant to be localized. This one should be. However, that means you'll have a problem with looking it up later. I think you may need to rethink how to do that.
-
-
reviewboard/static/rb/js/common.js (Diff revision 2) Comments should be proper sentences and end in a trailing period. (Yes, it's nitpicky, but we strive for nitpicky consistency :)
-
-
reviewboard/static/rb/js/common.js (Diff revision 2) Should use one var statement, and comma-separate them (one per line). This query should be more limited in scope. It should only check within the dashboard's datagrid, and check input[data-...]. Name should be "checkedBoxes"
-
-
-
reviewboard/static/rb/js/common.js (Diff revision 2) Same comment about vars. Also, blank line after vars.No need for the 'self' one though.
-
-
-
reviewboard/templates/reviews/dashboard_listview.html (Diff revision 2) One space indentation. I think it'd be best if these IDs were more specific so they can't conflict. Prefix with dashboard- I guess.
GC
Change Summary:
- Fixed comments from review - Added bug 579 - Added some screenshots
Bugs: |
|
||
|---|---|---|---|
Diff: |
Revision 3 (+102 -5) |
||
Added Files: |
-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/datagrids.py Ignored Files: reviewboard/static/rb/css/dashboard.less reviewboard/static/rb/js/common.js reviewboard/static/rb/js/datastore.js reviewboard/templates/reviews/dashboard_listview.html
-
I love the feature, but I'd like to address the UI a bit. The location and state of the command links (Submit Selected, Discard Selected) isn't optimal. It's confusing that they're in a position pretty far away from the check-boxes themselves (where the user's mouse is), and I don't think it looks very good to just have the text floating there in the header, especially when nothing is selected. Ideally, I'd like something to appear close to the mouse when the first box is checked (maybe slide out from the selection box in the column headers) and otherwise be hidden when there's nothing selected. Also, is it possible to put a check-box in the column selection drop-down? I think that would make it a lot more intuitive than just saying "Select".
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/datagrids.py Ignored Files: reviewboard/static/rb/css/dashboard.less reviewboard/static/rb/js/common.js reviewboard/static/rb/js/datastore.js reviewboard/templates/reviews/dashboard_listview.html