Add UI to archive and mute review requests with dashboard filters
Review Request #7086 — Created March 19, 2015 and submitted
This adds the following UI:
- archive and mute buttons in a Hide dropdown menu in a review request view
- banner that indicates that review request is archived with an unarchive button
- banner that indicates that review request is muted with an unmute button
Review request visibility is now passed as context data to templates. If visibility is VISIBLE, the Hide dropdown menu becomes visibile. If visibility is ARCHIVED or MUTED, the Hide dropdown menu is hidden and the appropriate archive or mute banner appears. When visibility is changed, a page refresh is triggered.
This also augments the UserSession WatchedItem (renamed StoredItem) to also include archivedReviewRequests and mutedReviewRequests. The only difference these objects are their URL and their error messages (addError and removeError methods were added in order to set different errors).
Archived and mute banner tests were added to reviewRequestEditorViewTests.js, all tests passed.
Description | From | Last Updated |
---|---|---|
Col: 80 E501 line too long (82 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (82 > 79 characters) |
reviewbot | |
Can you change this to be a single quoted string? |
brennie | |
This doesn't make sense grammatically. |
brennie | |
Undo this change. |
brennie | |
"a Hidden Items resource" |
brennie | |
(from David) I don't understand what the visibility attribute is used for. Can you explain? |
CR cristocrat | |
Remove this line. |
brennie | |
Single quotes for all these strings. |
brennie | |
(from David) Please use "review request" instead of "change" |
CR cristocrat | |
(from David) Please use "review request" instead of "change" |
CR cristocrat | |
Since we ignore the previous block for deciding the banner class, we should join these two blocks, with this new … |
brennie | |
'A' and 'M' should be named constants. |
brennie | |
Single quotes. This should be indented relative to the variable name. You can also do something like var reviewRequest = … |
brennie | |
We use a single var statement at the top of each function. |
brennie | |
See previous comment. |
brennie | |
See previous comment re: multiline strings. |
brennie | |
We use a single var statement at the top of each function. |
brennie | |
We don't wrap HTML at 80. |
brennie | |
We don't wrap HTML at 80. |
brennie | |
Based on your usage, having this be an empty dictionary isn't right. Probably it should be None |
david | |
I really don't like having this variable that third parties are expected to set. It's taken me several readings of … |
david | |
Blank line between these. |
chipx86 | |
Probably should be None. |
chipx86 | |
Can you move this value before the AJAX_SERIAL one? |
chipx86 | |
All var statements should go at the top of the file, like: `var Item, StoredItems; ... Item = ... StoredItems … |
chipx86 | |
Blank line between these. |
chipx86 | |
What's true? This should probably pass on all the arguments instead. It also needs to pass the correct context and … |
chipx86 | |
Same comments as above. |
chipx86 | |
I don't think these belong here. |
chipx86 | |
function() { should be aligned with the other arguments. Same below. |
chipx86 | |
I'll be mentioning this in the related review request, but the API shouldn't return the internal codes for these. Instead, … |
chipx86 | |
This doesn't seem right? |
chipx86 | |
Same. |
chipx86 | |
These are only used once in the function, so rather than assigning a local variable, I'd just use them directly … |
chipx86 | |
One variable per line. Also, initialized variables go before uninitialized variables. Same below. |
chipx86 | |
The localization parser won't handle the addition of strings properly. This needs to actually be one long string (even if … |
chipx86 |
- Summary:
-
Add UI to archive and mute review requests with dashboard filters.[WIP] Add UI to archive and mute review requests with dashboard filters.
- Diff:
-
Revision 2 (+278 -50)
-
Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/datagrids/grids.py Ignored Files: reviewboard/templates/base.html reviewboard/templates/reviews/review_header.html reviewboard/templates/datagrids/hideable_review_request_listview.html reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/reviewable_page_data.js reviewboard/static/rb/css/common.less reviewboard/templates/reviews/review_request_actions_secondary.html reviewboard/static/rb/js/models/userSessionModel.js Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/datagrids/grids.py Ignored Files: reviewboard/templates/base.html reviewboard/templates/reviews/review_header.html reviewboard/templates/datagrids/hideable_review_request_listview.html reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/reviewable_page_data.js reviewboard/static/rb/css/common.less reviewboard/templates/reviews/review_request_actions_secondary.html reviewboard/static/rb/js/models/userSessionModel.js
-
-
-
-
-
-
-
-
-
Since we ignore the previous block for deciding the banner class, we should join these two blocks, with this new block coming first and the prior block being
else-if
-ified. -
-
Single quotes. This should be indented relative to the variable name.
You can also do something like
var reviewRequest = this.model.get('reviewRequest'), confirmText = getText('Are you sure you want to archive this ' + 'review request?');
You can also use a list of strings a
.join
them together, but+
should be okay for just two strings. -
-
-
-
-
-
- Depends On:
-
- Diff:
Revision 3 (+244 -46)
-
Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py Ignored Files: reviewboard/templates/reviews/reviewable_page_data.js reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/review_request_actions_secondary.html reviewboard/static/rb/js/models/userSessionModel.js Tool: Pyflakes Processed Files: reviewboard/reviews/views.py Ignored Files: reviewboard/templates/reviews/reviewable_page_data.js reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/review_request_actions_secondary.html reviewboard/static/rb/js/models/userSessionModel.js
- Summary:
-
[WIP] Add UI to archive and mute review requests with dashboard filters.Add UI to archive and mute review requests with dashboard filters
- Diff:
-
Revision 4 (+350 -46)
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py Ignored Files: reviewboard/templates/base.html reviewboard/templates/reviews/reviewable_page_data.js reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/review_request_actions_secondary.html reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py Ignored Files: reviewboard/templates/base.html reviewboard/templates/reviews/reviewable_page_data.js reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/review_request_actions_secondary.html reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
-
Can you attach screenshots of the new UI?
-
-
I really don't like having this variable that third parties are expected to set. It's taken me several readings of the code to figure out what it meant. How about we just make a new argument (maybe called
trigger
) toaddImmediately
/removeImmediately
that causes it to trigger achanged
event (based on the listener, I don't see a reason to have two different events)?
- Description:
-
~ Added Archive and Mute buttons in a Hide dropdown menu in a review request view
~ This adds the following UI:
+ * archive and mute buttons in a Hide dropdown menu in a review request view + * banner that indicates that review request is archived with an unarchive button + * banner that indicates that review request is muted with an unmute button ~ Added button handlers that communicate with the webAPI.
~ Review request visibility is now passed as context data to templates. If visibility is VISIBLE, the Hide dropdown menu becomes visibile. If visibility is ARCHIVED or MUTED, the Hide dropdown menu is hidden and the appropriate archive or mute banner appears. When visibility is changed, a page refresh is triggered.
~ Augmented the UserSession WatchedItem (renamed StoredItem) to also include archivedReviewRequests and mutedReviewRequests. The only difference these objects are their URL and their error messages (addError and removeError methods were added in order to set different errors). Review request visibility is now passed as context data to templates.
~ This also augments the UserSession WatchedItem (renamed StoredItem) to also include archivedReviewRequests and mutedReviewRequests. The only difference these objects are their URL and their error messages (addError and removeError methods were added in order to set different errors).
- - Added dashboard button that toggles between showing and hiding hiden review requests.
- - Added banners for archiving and muting review requests.
- - Archived and Muted banners appear when a review request has been archived or muted. Each banner contains a button that can unarchive or unmute the review request. All of these events trigger a page refresh.
- Testing Done:
-
+ Archived and mute banner tests were added to reviewRequestEditorViewTests.js, all tests passed.
- Depends On:
-
- Groups:
- Diff:
Revision 5 (+362 -46)
- Added Files:
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py Ignored Files: reviewboard/templates/base.html reviewboard/templates/reviews/reviewable_page_data.js reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/review_request_actions_secondary.html reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py Ignored Files: reviewboard/templates/base.html reviewboard/templates/reviews/reviewable_page_data.js reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/review_request_actions_secondary.html reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
- Description:
-
~ This adds the following UI:
~ * archive and mute buttons in a Hide dropdown menu in a review request view ~ * banner that indicates that review request is archived with an unarchive button ~ * banner that indicates that review request is muted with an unmute button ~ This adds the following UI:
~ ~ - archive and mute buttons in a Hide dropdown menu in a review request view
~ - banner that indicates that review request is archived with an unarchive button
+ - banner that indicates that review request is muted with an unmute button
Review request visibility is now passed as context data to templates. If visibility is VISIBLE, the Hide dropdown menu becomes visibile. If visibility is ARCHIVED or MUTED, the Hide dropdown menu is hidden and the appropriate archive or mute banner appears. When visibility is changed, a page refresh is triggered.
This also augments the UserSession WatchedItem (renamed StoredItem) to also include archivedReviewRequests and mutedReviewRequests. The only difference these objects are their URL and their error messages (addError and removeError methods were added in order to set different errors).
-
-
-
-
-
All
var
statements should go at the top of the file, like:`var Item,
StoredItems;...
Item = ...
StoredItems = ...
``` -
-
What's
true
? This should probably pass on all the arguments instead. It also needs to pass the correct context and check thatoptions.success
is set:if (options && _.isFunction(options.success)) { options.success.apply(context, arguments); }
-
-
-
-
I'll be mentioning this in the related review request, but the API shouldn't return the internal codes for these. Instead, it should return boolean fields for archived vs. muted, probably. See my other review :)
-
-
-
These are only used once in the function, so rather than assigning a local variable, I'd just use them directly below.
-
-
The localization parser won't handle the addition of strings properly. This needs to actually be one long string (even if that means going over the 79 character limit).
Same below.
- Diff:
-
Revision 6 (+366 -46)
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py Ignored Files: reviewboard/templates/base.html reviewboard/templates/reviews/reviewable_page_data.js reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/review_request_actions_secondary.html reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py Ignored Files: reviewboard/templates/base.html reviewboard/templates/reviews/reviewable_page_data.js reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/review_request_actions_secondary.html reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js