-
-
-
reviewboard/reviews/detail.py (Diff revision 1) Missing period.
Can this go into what this class does?
Also needs
Attributes:. -
-
reviewboard/reviews/detail.py (Diff revision 1) Can we use
~before the class reference, so it just shows up as "StatusUpdate" in the docs? -
-
-
-
reviewboard/reviews/models/status_update.py (Diff revision 1) Properties before normal methods.
The model shouldn't be handling any HTML-related data, though. This should be done by the view and probably entry stuff.
-
-
-
reviewboard/reviews/views.py (Diff revision 1) Mind using keyword arguments herE? It'd help with making the length check more clear.
-
reviewboard/reviews/views.py (Diff revision 1) How about just making
finalize()part of the base API for these entries, so it's not treated specially? That way it's documented that any subclass can perform operations there. -
-
reviewboard/static/rb/js/views/changeBoxView.es6.js (Diff revision 1) We have checking for
#reviewin three places now. It'd be nice to have this handled in one common place somehow. Maybe a utility function onReviewBoxViewor something that returns the ID for the review provided in the URL, or something. Just so we're not duplicating ourselves here and having to potentially fix something in two places down the road.(Ideally, we'd have something using routers and some way to register a handler, but that's perhaps a bit much for now.)
-
reviewboard/static/rb/js/views/changeBoxView.es6.js (Diff revision 1) Can you put parens around the comparison? With fat arrow functions, I always have to re-read these expressions (especially since Python would allow very similar statements, at first glance).
-
reviewboard/static/rb/js/views/changeBoxView.es6.js (Diff revision 1) Some of this results in duplicate logic between here and review boxes, particularly for the values/CSS properties dealing with the animation when adding/removing labels.
It'd be nice instead to take this bit and just turn it into a view (like a
RB.BoxIssuesLabelViewor something) that has methods for showing/hiding and setting text. -
reviewboard/static/rb/js/views/initialStatusUpdatesBoxView.es6.js (Diff revision 1) Should be standard one-line summary, multi-line description.
-
reviewboard/static/rb/js/views/initialStatusUpdatesBoxView.es6.js (Diff revision 1) I'm thinking we really want this (and the
this._reviewViews = ...) split off into some mixin. -
reviewboard/staticbundles.py (Diff revision 1) Can we move up by
issueSummaryTableView, to try to keep the rough alphabetical order? (I know we can't always do that, but if it's possible here, it'd be nice.) -
reviewboard/templates/reviews/boxes/change.html (Diff revision 1) Here and further down,
<h1>is probably not appropriate. The primary<h1>is the app title in the header bar. The box title, while not a<hX>, should be considered header level 2. This should probably be<h3>.Same with others below.
-
reviewboard/templates/reviews/boxes/change.html (Diff revision 1) This can also use
{% include ... with ... %} -
reviewboard/templates/reviews/boxes/initial_status_updates.html (Diff revision 1) Looks like we're duplicating a lot of complex stuff here. We should split this off into a utility template.
-
reviewboard/templates/reviews/boxes/initial_status_updates.html (Diff revision 1) Same comments about
<h1>vs.<h3>. -
reviewboard/templates/reviews/boxes/initial_status_updates.html (Diff revision 1) Can be
{% include ... with ... %} -
reviewboard/templates/reviews/boxes/initial_status_updates.js (Diff revision 1) This is done twice. Can we consolidate?
-
reviewboard/templates/reviews/review_detail.html (Diff revision 1) This can be:
{% include entry.template_name with entry=initial_status.entry %} -
Status Updates part 8: User Interface.
Review Request #8409 — Created Sept. 18, 2016 and submitted
This change adds the UI for status updates in the review request page. Status
updates are shown in two places:
- If a status update is associated with a
ChangeDescription, it will be shown
in the entry for that change description. - Otherwise, it's assumed that the status update is for the initial diff (or any
other attachments, etc. that were present when the review request was first
published). These are shown in a new entry which always appears immediately
below the review request box.
The one thing that's missing from this is the draft banners when replying to
any comments in the reviews associated with status updates. This is because
multiple reviews can be aggregated together into one entry, and we don't yet
have the ability to deal with that. Our proposed redesign for the draft banners
will solve this (since there will always be one banner at the top of the page).
If the reviews.status_updates feature is not enabled (which is currently the
default), any reviews associated with status updates will be displayed just
like all the other reviews, in their own entries.
- Created a variety of status updates and checked that they rendered correctly.
- Tested the default expand/collapse state for the new initial status updates
entry (expanded only when there are no change descriptions or draft replies). - Disabled the feature in my settings and saw that all the reviews were now
shown as regular review entries.
| Description | From | Last Updated |
|---|---|---|
|
Missing period. |
 chipx86
|
|
|
Missing period. Can this go into what this class does? Also needs Attributes:. |
chipx86
|
|
|
Could probably just use a collections.Counter. |
chipx86
|
|
|
Can we use ~ before the class reference, so it just shows up as "StatusUpdate" in the docs? |
chipx86
|
|
|
Here, too. |
chipx86
|
|
|
Blank line between these. |
chipx86
|
|
|
Blank line between these. |
chipx86
|
|
|
Properties before normal methods. The model shouldn't be handling any HTML-related data, though. This should be done by the view … |
chipx86
|
|
|
Same here. |
chipx86
|
|
|
Won't Review always have a status_update attribute? |
chipx86
|
|
|
Mind using keyword arguments herE? It'd help with making the length check more clear. |
chipx86
|
|
|
How about just making finalize() part of the base API for these entries, so it's not treated specially? That way … |
chipx86
|
|
|
Two blank lines. |
chipx86
|
|
|
We have checking for #review in three places now. It'd be nice to have this handled in one common place … |
chipx86
|
|
|
Can you put parens around the comparison? With fat arrow functions, I always have to re-read these expressions (especially since … |
chipx86
|
|
|
Some of this results in duplicate logic between here and review boxes, particularly for the values/CSS properties dealing with the … |
chipx86
|
|
|
Should be standard one-line summary, multi-line description. |
chipx86
|
|
|
I'm thinking we really want this (and the this._reviewViews = ...) split off into some mixin. |
chipx86
|
|
|
Can we move up by issueSummaryTableView, to try to keep the rough alphabetical order? (I know we can't always do … |
chipx86
|
|
|
Here and further down, <h1> is probably not appropriate. The primary <h1> is the app title in the header bar. … |
chipx86
|
|
|
This can also use {% include ... with ... %} |
chipx86
|
|
|
Looks like we're duplicating a lot of complex stuff here. We should split this off into a utility template. |
chipx86
|
|
|
Same comments about <h1> vs. <h3>. |
chipx86
|
|
|
Can be {% include ... with ... %} |
chipx86
|
|
|
This is done twice. Can we consolidate? |
chipx86
|
|
|
This can be: {% include entry.template_name with entry=initial_status.entry %} |
chipx86
|
|
|
Here, too. |
chipx86
|
|
|
bool |
chipx86
|
|
|
Can you add a trailing period here? |
chipx86
|
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+609 -37) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/detail.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/boxes/review.html reviewboard/templates/reviews/boxes/initial_status_updates.html reviewboard/static/rb/js/views/changeBoxView.es6.js reviewboard/templates/reviews/status_update_summary.html reviewboard/templates/reviews/boxes/change.js reviewboard/static/rb/js/views/reviewView.es6.js reviewboard/templates/reviews/review_detail.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/boxes/initial_status_updates.js reviewboard/static/rb/js/views/initialStatusUpdatesBoxView.es6.js reviewboard/templates/reviews/boxes/change.html reviewboard/static/rb/js/views/reviewBoxView.es6.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/detail.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/boxes/review.html reviewboard/templates/reviews/boxes/initial_status_updates.html reviewboard/static/rb/js/views/changeBoxView.es6.js reviewboard/templates/reviews/status_update_summary.html reviewboard/templates/reviews/boxes/change.js reviewboard/static/rb/js/views/reviewView.es6.js reviewboard/templates/reviews/review_detail.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/boxes/initial_status_updates.js reviewboard/static/rb/js/views/initialStatusUpdatesBoxView.es6.js reviewboard/templates/reviews/boxes/change.html reviewboard/static/rb/js/views/reviewBoxView.es6.js
Change Summary:
Fix a few issues discovered during further testing:
- CSS rules for the icons were translated wrong from the fa CSS.
- Typo in the state summary calculations.
- Collapsed change descriptions still showed the status updates.
- body_top and body_bottom are now displayed in status updates if they're present. Behavior for reviews (where body_top is always displayed) is unchanged.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+671 -85) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/detail.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/boxes/review.html reviewboard/templates/reviews/boxes/initial_status_updates.html reviewboard/templates/reviews/boxes/review_body.html reviewboard/static/rb/js/views/changeBoxView.es6.js reviewboard/templates/reviews/status_update_summary.html reviewboard/templates/reviews/boxes/change.js reviewboard/static/rb/js/views/reviewView.es6.js reviewboard/templates/reviews/review_detail.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/boxes/initial_status_updates.js reviewboard/static/rb/js/views/initialStatusUpdatesBoxView.es6.js reviewboard/templates/reviews/boxes/change.html reviewboard/static/rb/js/views/reviewBoxView.es6.js reviewboard/templates/reviews/status_update_review_section.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/detail.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/boxes/review.html reviewboard/templates/reviews/boxes/initial_status_updates.html reviewboard/templates/reviews/boxes/review_body.html reviewboard/static/rb/js/views/changeBoxView.es6.js reviewboard/templates/reviews/status_update_summary.html reviewboard/templates/reviews/boxes/change.js reviewboard/static/rb/js/views/reviewView.es6.js reviewboard/templates/reviews/review_detail.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/boxes/initial_status_updates.js reviewboard/static/rb/js/views/initialStatusUpdatesBoxView.es6.js reviewboard/templates/reviews/boxes/change.html reviewboard/static/rb/js/views/reviewBoxView.es6.js reviewboard/templates/reviews/status_update_review_section.html