-
-
reviewboard/reviews/views.py (Diff revision 1) list comprehension redefines 'file_attachment' from line 587
Diff fragments in review comments can now be expanded and collapsed.
Review Request #6380 — Created Sept. 28, 2014 and submitted
Diff fragments can now be expanded in three ways:
- expanded above or below the current context by a fixed number of
lines (20);
- expanded up to the previous header (i.e., method definition); or
- expanded completely above or below.
After a diff fragment is expanded in one direction, it can be
collapsed via a floating button that follows the viewport. If there is
still more of the diff to be expanded, it may be expanded again.The
comment_diff_fragments
view now takes an optional URL query
parameter,lines-of-context
, which indicates the number of lines
above and below that should be included in the fragment. Likewise,
thebuild_diff_comment_fragments
now takes alines_of_context
parameter and uses that to calculate thefirst_line
and
num_lines
parameters forget_file_chunks_in_range
.The activity indicator functionality in
RB.apiCall
(in
apiUtils.js
) has been refactored into another method in the same
file:RB.setActivityIndicator
. Asynchronous activity that does not
involve API calls can use this method to enable or disable the
activity indicator.Add the
expand_fragment_link
andexpand_fragment_header_link
Django template tags. These tags create expansion links for use in
diff fragments. These functions use a new template:
reviews/expand_link.html
.Added Brian Cherne's hoverIntent jQuery plugin to the 3rd party static
bundles.The
DiffFragmentQueueView
(diffFragmentQueueView.js
) now handles
the expansion and contraction of diff fragments. This view monitors
for clicks on expansion and contraction buttons and dynamically adds
a script tag which will replace the contents of the appropriate diff
fragment with the same fragment, but with expanded (or contracted)
context. The activity indicator is turned on when a diff fragment is
requested and off when the requested fragment has been received.The expansion links for diff fragments are only shown when the mouse
is hovering over the comment container for that fragment. Once the
mouse moves off the comment container, the expansion links will become
hidden. The hoverIntent jQuery plugin ensures that clumsy mouse
movements will not result in the controls toggling on and off
repeatedly.The
ReviewBoxListView
now constructs theDiffFragmentQueueView
with an element (#container
) on which it should listen for events.The diff fragment template (
diff_comment_fragment.html
) has been
modified to only show 2 columns (instead of 4) when the file in the
diff is a new file. This change makes the diff comment fragment viewer
look more similar to the diffviewer.When a diff fragment is expanded to have context, the lines that have
been flagged for review will be surrounded above and below by a gray
border.
Unit tests pass.
Verified the following in Chrome and Firefox:
- Expansion links are only visible if the diff fragment can be
expanded in that direction. - When a review request is loaded, the expansion controls are hidden.
- Upon hovering over a diff fragment, the expansion controls expand.
- Upon stopping hovering over a diff fragment, this is a delay before
the expansion controls become hidden again. - Upon stopping hovering over a diff fragment, if the mouse hovers
over the diff fragment again before the delay expires, the
expansion controls do not become hidden. - Clicking on the "expand by 20" lines buttons expands the context by
20 lines in the appropriate direction. - Click on the expand to header button expands to that header.
- Expanding multiple times expands upon the current context.
- The collapse button smoothly follows scrolling, staying in the
centre of the screen (if the fragment takes up the whole view) or
the centre of the fragment (if the fragment does not take up the
whole view). - Clicking the collapse button makes all context hidden and removes
the collapse button. - The lines originally flagged for review have a border around them.
- When the file is a new file, only one line number column is
visible. - If upon contracting a diff fragment the mouse is no longer above
the fragment for a period, the diff fragment's controls will become
hidden.
Also verified the following:
- An invalid
lines_of_context
query parameter is interpreted as
0 lines of context above and below - If too many
lines_of_context
query parameter values are passed
(e.g.,0,0,0,0
), it is interpreted as if only the first two
values were present. - If the
lines_of_context
query parameter is passed with no value,
it is interpreted as0,0
- If the
lines_of_context
query parameter is passed with only one
value, it is interpreted as both the above and below context.
Description | From | Last Updated |
---|---|---|
I think the thickness of the border is a little much, was 1px not visible enough? |
SM smacleod | |
list comprehension redefines 'file_attachment' from line 587 |
reviewbot | |
list comprehension redefines 'file_attachment' from line 588 |
reviewbot | |
list comprehension redefines 'file_attachment' from line 588 |
reviewbot | |
Col: 24 E231 missing whitespace after ',' |
reviewbot | |
list comprehension redefines 'file_attachment' from line 588 |
reviewbot | |
list comprehension redefines 'file_attachment' from line 588 |
reviewbot | |
list comprehension redefines 'file_attachment' from line 589 |
reviewbot | |
list comprehension redefines 'file_attachment' from line 590 |
reviewbot | |
'mark_safe' imported but unused |
reviewbot | |
'render_markdown' imported but unused |
reviewbot | |
Col: 25 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 52 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 52 E128 continuation line under-indented for visual indent |
reviewbot | |
Add a trailing comma. |
david | |
If you call mark_safe() here, you can avoid the |safe in the template. This should probably also be named something … |
david | |
Dictionary entries in python should always end in a trailing comma. |
david | |
Add a trailing comma. |
david | |
This should probably be wrapped in a try/except in order to avoid 500s if a bogus query string is passed. |
david | |
Should have a trailing comma. |
david | |
Can we add a non-minified version of this instead? When we compile it we'll minify, but having the original source … |
david | |
Where do these numbers come from? |
david | |
Can you name this $activityIndicator? |
david | |
Single-line comments (other than those used as method comments) should use // |
david | |
All blocks should use {} |
david | |
We'd typically indent this as: $chunks = $button .closest('.sidebyside') .children('tbody') .not('.diff-header'); |
david | |
Single-line comments should use //. Here and below. |
david | |
These should be on the same line. |
david | |
These should be on the same line. |
david | |
setActivityIndicator checks RB.ajaxOptions.enableIndicator so you shouldn't need the conditional. |
david | |
Should have a period at the end. |
david | |
{} |
david | |
typo: finishd |
david | |
You shouldn't need the conditional here because setActivityIndicator checks RB.ajaxOptions.enableIndicator. |
david | |
Variable definitions should happen at the top of the method. |
david | |
Multi-line comments should be formatted as: /* * text */ |
david | |
Same here re: formatting. Also should end in a period. |
david | |
Can you revert this change? |
david | |
Just to be safe, how about we escape the 'text' field here? You can use format_html to create HTML and … |
david | |
Generally, in Python it's best to avoid lists in default arguments because any modification to that variable can end up … |
david | |
What happens if this URL is loaded with lines_of_context= ? |
david | |
This should use an === comparison. |
david | |
Should have a blank /* on the first line of the comment. |
david | |
You don't need to pass $expanded in to this--it will be available in the closure. |
david | |
Variable definitions should all be at the top of their method. |
david | |
Blocks should always use {} |
david | |
Col: 33 E225 missing whitespace around operator |
reviewbot | |
You don't need to call escape(text) because format_html will escape any arguments that aren't already safe. |
david | |
Can you add an "else: raise ValueError" at the end of this? |
david |
Change Summary:
Add extra line to comply with PEP8.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+15 -4) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py
-
reviewboard/reviews/views.py (Diff revision 2) list comprehension redefines 'file_attachment' from line 588
Change Summary:
Remove extra blank line after a comment.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+14 -4) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py
-
reviewboard/reviews/views.py (Diff revision 3) list comprehension redefines 'file_attachment' from line 588
Change Summary:
Change
before_lines
andafter_lines
to belines_of_context
(for arguments)
andlines-of-context
(for query parameter
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 4 (+19 -4) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py
-
-
reviewboard/reviews/views.py (Diff revision 4) list comprehension redefines 'file_attachment' from line 588
Change Summary:
fix space after comma
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+19 -4) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py Tool: Pyflakes Processed Files: reviewboard/reviews/views.py
-
reviewboard/reviews/views.py (Diff revision 5) list comprehension redefines 'file_attachment' from line 588
Change Summary:
Add client side functionality for diff expansion.
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 6 (+56 -6) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py Ignored Files: reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/views/diffFragmentQueueView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py Ignored Files: reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/views/diffFragmentQueueView.js
-
reviewboard/reviews/views.py (Diff revision 6) list comprehension redefines 'file_attachment' from line 589
Change Summary:
Formatting
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+57 -5) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py Ignored Files: reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/views/diffFragmentQueueView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py Ignored Files: reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/views/diffFragmentQueueView.js
-
reviewboard/reviews/views.py (Diff revision 7) list comprehension redefines 'file_attachment' from line 590
Change Summary:
Add activity indicator interaction.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 8 (+99 -20) |
Change Summary:
Use the lines-of-context parameter to determine when we should add JS to turn off the activity indicator
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+97 -20) |
Change Summary:
Code cleanup -- better JS and Python. Provide semantic conditions to the Django template in build_diff_comment_fragment. Improve LAF of expansion links.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 10 (+174 -27) |
Change Summary:
Almost everything works. Just need to fix collapsable buttons.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 11 (+410 -143) |
Change Summary:
Collapsible diff segments. Move out of WIP status. Add screenshots.
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 12 (+293 -31) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
Change Summary:
Base off latest master.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 13 (+290 -36) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/templates/diffviewer/expand_link.html reviewboard/static/rb/js/views/diffFragmentQueueView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/templates/diffviewer/expand_link.html reviewboard/static/rb/js/views/diffFragmentQueueView.js
-
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 13) 'render_markdown' imported but unused
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 13) Col: 25 E128 continuation line under-indented for visual indent
-
reviewboard/reviews/views.py (Diff revision 13) Col: 52 E128 continuation line under-indented for visual indent
-
reviewboard/reviews/views.py (Diff revision 13) Col: 52 E128 continuation line under-indented for visual indent
Change Summary:
Update to fix formatting errors. Move back into WIP status.
Summary: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 14 (+289 -37) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/templates/diffviewer/expand_link.html reviewboard/static/rb/js/views/diffFragmentQueueView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/templates/diffviewer/expand_link.html reviewboard/static/rb/js/views/diffFragmentQueueView.js
Change Summary:
Improve UI.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 15 (+520 -151) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/reviews/boxes/review.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/js/utils/collapseButtonManager.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/lib/js/jquery.hoverIntent.min.js reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/templates/diffviewer/expand_link.html reviewboard/static/rb/js/views/diffFragmentQueueView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/reviews/boxes/review.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/js/utils/collapseButtonManager.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/lib/js/jquery.hoverIntent.min.js reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/templates/diffviewer/expand_link.html reviewboard/static/rb/js/views/diffFragmentQueueView.js
Change Summary:
Update indentation in apiUtils.js
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 16 (+520 -151) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/reviews/boxes/review.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/js/utils/collapseButtonManager.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/lib/js/jquery.hoverIntent.min.js reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/templates/diffviewer/expand_link.html reviewboard/static/rb/js/views/diffFragmentQueueView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/reviews/boxes/review.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/js/utils/collapseButtonManager.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/lib/js/jquery.hoverIntent.min.js reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/templates/diffviewer/expand_link.html reviewboard/static/rb/js/views/diffFragmentQueueView.js
Change Summary:
Update color of comment divider
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 17 (+521 -151) |
||||
Removed Files: |
|||||
Added Files: |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/reviews/boxes/review.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/js/utils/collapseButtonManager.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/lib/js/jquery.hoverIntent.min.js reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/templates/diffviewer/expand_link.html reviewboard/static/rb/js/views/diffFragmentQueueView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/reviews/boxes/review.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/js/utils/collapseButtonManager.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/lib/js/jquery.hoverIntent.min.js reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/templates/diffviewer/expand_link.html reviewboard/static/rb/js/views/diffFragmentQueueView.js
Change Summary:
Fix regression in diffviewer.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 18 (+518 -149) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/reviews/boxes/review.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/js/utils/collapseButtonManager.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/lib/js/jquery.hoverIntent.min.js reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/templates/diffviewer/expand_link.html reviewboard/static/rb/js/views/diffFragmentQueueView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/reviews/boxes/review.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/js/utils/collapseButtonManager.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/lib/js/jquery.hoverIntent.min.js reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/templates/diffviewer/expand_link.html reviewboard/static/rb/js/views/diffFragmentQueueView.js
Change Summary:
Fix jumpy button. Move back out of WIP status.
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 19 (+502 -40) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/reviews/boxes/review.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/lib/js/jquery.hoverIntent.min.js reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/templates/diffviewer/expand_link.html reviewboard/static/rb/js/views/diffFragmentQueueView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/reviews/boxes/review.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/lib/js/jquery.hoverIntent.min.js reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/templates/diffviewer/expand_link.html reviewboard/static/rb/js/views/diffFragmentQueueView.js
-
I'd like a lot more detail on the test cases that you ran through, especially listing out any edge cases that you've tested (for example, what about comments at the beginning of a file? The end? A comment on a file that's only a single line?)
-
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 19) If you call
mark_safe()
here, you can avoid the |safe in the template. This should probably also be named something like 'text_html' in order to clarify that it's HTML and not plain text. -
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 19) Dictionary entries in python should always end in a trailing comma.
-
-
reviewboard/reviews/views.py (Diff revision 19) This should probably be wrapped in a try/except in order to avoid 500s if a bogus query string is passed.
-
-
reviewboard/static/lib/js/jquery.hoverIntent.min.js (Diff revision 19) Can we add a non-minified version of this instead? When we compile it we'll minify, but having the original source will make it easier to debug for development.
I'm planning on migrating more of our 3rd-party code to using non-minified sources in the future.
-
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 19) Where do these numbers come from?
-
-
reviewboard/static/rb/js/views/diffFragmentQueueView.js (Diff revision 19) Single-line comments (other than those used as method comments) should use //
-
-
reviewboard/static/rb/js/views/diffFragmentQueueView.js (Diff revision 19) We'd typically indent this as:
$chunks = $button .closest('.sidebyside') .children('tbody') .not('.diff-header');
-
reviewboard/static/rb/js/views/diffFragmentQueueView.js (Diff revision 19) Single-line comments should use //. Here and below.
-
reviewboard/static/rb/js/views/diffFragmentQueueView.js (Diff revision 19) These should be on the same line.
-
reviewboard/static/rb/js/views/diffFragmentQueueView.js (Diff revision 19) These should be on the same line.
-
reviewboard/static/rb/js/views/diffFragmentQueueView.js (Diff revision 19) setActivityIndicator checks RB.ajaxOptions.enableIndicator so you shouldn't need the conditional.
-
reviewboard/static/rb/js/views/diffFragmentQueueView.js (Diff revision 19) Should have a period at the end.
-
-
-
reviewboard/static/rb/js/views/diffFragmentQueueView.js (Diff revision 19) You shouldn't need the conditional here because
setActivityIndicator
checksRB.ajaxOptions.enableIndicator
. -
reviewboard/static/rb/js/views/diffFragmentQueueView.js (Diff revision 19) Variable definitions should happen at the top of the method.
-
reviewboard/static/rb/js/views/diffFragmentQueueView.js (Diff revision 19) Multi-line comments should be formatted as:
/* * text */
-
reviewboard/static/rb/js/views/diffFragmentQueueView.js (Diff revision 19) Same here re: formatting. Also should end in a period.
-
Change Summary:
Address the issues in David's review.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 20 (+620 -43) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/lib/js/jquery.hoverIntent.js reviewboard/templates/reviews/boxes/review.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/static/rb/js/views/diffFragmentQueueView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/lib/js/jquery.hoverIntent.js reviewboard/templates/reviews/boxes/review.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/static/rb/js/views/diffFragmentQueueView.js
-
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 20) Just to be safe, how about we escape the 'text' field here? You can use format_html to create HTML and escape all in one go.
-
reviewboard/reviews/views.py (Diff revision 20) Generally, in Python it's best to avoid lists in default arguments because any modification to that variable can end up changing your default argument for the next call. The usual pattern is:
def method(x=None): if x is None: x = [...]
That way you can later modify x without causing any strange bugs.
-
reviewboard/reviews/views.py (Diff revision 20) What happens if this URL is loaded with
lines_of_context=
? -
reviewboard/static/rb/js/views/diffFragmentQueueView.js (Diff revision 20) This should use an === comparison.
-
reviewboard/static/rb/js/views/diffFragmentQueueView.js (Diff revision 20) Should have a blank
/*
on the first line of the comment. -
reviewboard/static/rb/js/views/diffFragmentQueueView.js (Diff revision 20) You don't need to pass $expanded in to this--it will be available in the closure.
-
reviewboard/static/rb/js/views/diffFragmentQueueView.js (Diff revision 20) Variable definitions should all be at the top of their method.
-
reviewboard/static/rb/js/views/diffFragmentQueueView.js (Diff revision 20) Blocks should always use {}
Change Summary:
Fix issues in David's review.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 21 (+630 -44) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/lib/js/jquery.hoverIntent.js reviewboard/templates/reviews/boxes/review.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/static/rb/js/views/diffFragmentQueueView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/lib/js/jquery.hoverIntent.js reviewboard/templates/reviews/boxes/review.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/static/rb/js/views/diffFragmentQueueView.js
-
Change Summary:
Formatting
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 22 (+630 -44) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/lib/js/jquery.hoverIntent.js reviewboard/templates/reviews/boxes/review.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/static/rb/js/views/diffFragmentQueueView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/lib/js/jquery.hoverIntent.js reviewboard/templates/reviews/boxes/review.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/static/rb/js/views/diffFragmentQueueView.js
-
Can you add bug 2041?
I'd still like to see a lot more detail in your testing done.
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 22) You don't need to call escape(text) because format_html will escape any arguments that aren't already safe.
Change Summary:
Fix format_html call. TODO: Add more testing info.
Bugs: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 23 (+630 -44) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/lib/js/jquery.hoverIntent.js reviewboard/templates/reviews/boxes/review.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/static/rb/js/views/diffFragmentQueueView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/lib/js/jquery.hoverIntent.js reviewboard/templates/reviews/boxes/review.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/static/rb/js/views/diffFragmentQueueView.js
-
One last comment on the code:
-
reviewboard/reviews/views.py (Diff revision 23) Can you add an "else: raise ValueError" at the end of this?
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 24 (+632 -44) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/lib/js/jquery.hoverIntent.js reviewboard/templates/reviews/boxes/review.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/static/rb/js/views/diffFragmentQueueView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/lib/js/jquery.hoverIntent.js reviewboard/templates/reviews/boxes/review.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/static/rb/js/views/diffFragmentQueueView.js
Change Summary:
Fix regression where fragments wouldnt expand.
Make controls hidden immediately instead of slide up on page load.
Add testing info.
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 25 (+633 -44) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/lib/js/jquery.hoverIntent.js reviewboard/templates/reviews/boxes/review.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/static/rb/js/views/diffFragmentQueueView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/expand_link.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/lib/js/jquery.hoverIntent.js reviewboard/templates/reviews/boxes/review.html reviewboard/static/rb/js/views/reviewBoxListView.js reviewboard/static/rb/js/utils/apiUtils.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/diff_comment_fragment.html reviewboard/static/rb/js/views/diffFragmentQueueView.js
Change Summary:
Fix typo
Testing Done: |
|
---|