[WIP] Add ability to embed per file diffs on external sites
Review Request #7890 — Created Jan. 17, 2016 and discarded
| Information | |
|---|---|
| samchurney | |
| Review Board | |
| master | |
| Reviewers | |
| reviewboard | |
Currently, Review Board's diffs can only be viewed within Review
Board. Sometimes, however, it would be nice to take a diff and
show it on another webpage, muchlike you can with GitHub's Gists
service, or a YouTube video.This change creates an endpoint for the individual FileDiffs
from the DiffViewer for an iframe to point to. E.g.:reviews.reviewboard.org/r/(review_request_id)/diff/(revision)/embed/(filediff_id)/
When this endpoint is hit, the necessary files to display a single
Diff fragment are returned as if it were extracted from the DiffViewer.This change also includes a chain link icon beside the filename
in the diff viewer which, when clicked, shows a box with
pre-filled input fields which will hold embed and permalinks to
that file diff.The full URL is generated and placed in the diff viewer dropdown box
Previously, the path was only included and now the full URL, with
the HTTP host, is included.A new template has been created to display a diff fragment instead
of using the diff fragment template from the diff viewer. This will
save on loading more static files than necessary for the embedded
diff fragment and provides more control.The proper styles have been applied to the diff fragment embed so
the embedded diff now looks presentable and in-line with the RB
theme.The original diff fragment view and the diff fragment embed view
shared a lot of the same code. Similar code between these two
class based views were extracted into a parent class.A link was also added to the embedded diff fragment that easily
allows a user to view the full diff on review board.
- Tested URI in browser with no ill side effects.
- Embed box needs to be fixed in Safari. Timeout is too fast.
| Description | From | Last Updated |
|---|---|---|
|
Can we maybe add some cute link back to the main diff that says something like "View this change on … |
 david
|
|
|
These fonts seem... large. |
david
|
|
|
Col: 1 E302 expected 2 blank lines, found 1 |
 reviewbot
|
|
|
Col: 80 E501 line too long (82 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (101 > 79 characters) |
reviewbot
|
|
|
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
Col: 28 W291 trailing whitespace |
reviewbot
|
|
|
Col: 30 W291 trailing whitespace |
reviewbot
|
|
|
Can you format this so that each parameter is not on its own line? |
 brennie
|
|
|
Function-based views are on their way out, and we should probably not write new ones. Can we make this a … |
david
|
|
|
Docstrings should be of the form: """Single line summary. Multi-line description. """ This is also missing an Args and Returns … |
brennie
|
|
|
We use the imperitive mood for doc strings and doc comment, so this should read as: "Initialize the file diff … |
brennie
|
|
|
We use the imperitive mood for doc strings and doc comment, so this should read as: "Initialize the file diff … |
brennie
|
|
|
We can dispense with the whole router thing since users won't be able to change what revision they're looking at. … |
david
|
|
|
"Remove" |
brennie
|
|
|
"Render" |
brennie
|
|
|
render() should return this;. |
brennie
|
|
|
Put constants at the beginning |
brennie
|
|
|
This doesn't do anything. |
brennie
|
|
|
Col: 60 W291 trailing whitespace |
reviewbot
|
|
|
'DiffViewerView' imported but unused |
reviewbot
|
|
|
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
Col: 80 E501 line too long (87 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 5 E303 too many blank lines (2) |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 9 E122 continuation line missing indentation or outdented |
reviewbot
|
|
|
Col: 13 E122 continuation line missing indentation or outdented |
reviewbot
|
|
|
Col: 20 E251 unexpected spaces around keyword / parameter equals |
reviewbot
|
|
|
Col: 19 E702 multiple statements on one line (semicolon) |
reviewbot
|
|
|
local variable 'diffset' is assigned to but never used |
reviewbot
|
|
|
Col: 9 E122 continuation line missing indentation or outdented |
reviewbot
|
|
|
Col: 19 E702 multiple statements on one line (semicolon) |
reviewbot
|
|
|
'build_server_url' imported but unused |
reviewbot
|
|
|
These should all be sorted in alphabetical order. |
david
|
|
|
Undo this line deletion. |
david
|
|
|
Undo this line addition. |
david
|
|
|
This seems to share a lot with DiffFragmentView. Can we create some intermediate parent class for both that includes the … |
david
|
|
|
If there's no docstring, the first line can be immediately after the class ... line. But maybe it's better to … |
david
|
|
|
No blank line here (and/or add a docstring). |
david
|
|
|
Can we log the exception as well? |
david
|
|
|
Can we log the exception as well? |
david
|
|
|
No blank line and/or add a docstring. |
david
|
|
|
I know this is just copy/pasted, but we're trying to move towards using the imperative mood for docstrings as per … |
david
|
|
|
Mind adding "Args:" and "Returns:" sections as per https://www.reviewboard.org/docs/codebase/dev/ ? |
david
|
|
|
Mind adding "Args:" and "Returns:" sections as per https://www.reviewboard.org/docs/codebase/dev/ ? |
david
|
|
|
Should be "Fetch ..." |
david
|
|
|
Mind adding "Args:" and "Returns:" sections as per https://www.reviewboard.org/docs/codebase/dev/ ? |
david
|
|
|
No blank line and/or docstring. |
david
|
|
|
No blank line here. |
david
|
|
|
No blank line here, docstring. |
david
|
|
|
Can you put this on the previous line? name="diff-fragment-view"), |
david
|
|
|
Should go on the previous line. |
david
|
|
|
Please put this line back. |
david
|
|
|
Remove this line. |
david
|
|
|
Remove this line. |
david
|
|
|
Can you make this alphabetical-ish? |
david
|
|
|
Trailing whitespace. |
david
|
|
|
Alphabetical-ish? |
david
|
|
|
Trailing whitespace. |
david
|
|
|
Why this change? Something seems wrong. |
david
|
|
|
I know fontawesome says to use <i> but we generally use <span> instead. |
david
|
|
|
Remove this line. |
david
|
|
|
Can you make this one big var statement with individual variables separated by commas? |
david
|
|
|
Since these are jquery objects, let's prefix the variable name with $ |
david
|
|
|
Space between if and (. We probably should also use embedBox.is(':visible') instead of checking the CSS value. |
david
|
|
|
.css('display', 'block') is equivalent to .show() |
david
|
|
|
This is the same as embedBox.hide() |
david
|
|
|
Please define a constant for the 5000. |
david
|
|
|
embedBox.hide() |
david
|
|
|
Trailing whitespace. |
david
|
|
|
We format these like: embedBox .on('mouseenter' function() { // ... }) .on('mouseleave', function() { // ... }); |
david
|
|
|
Space between function() and { |
david
|
|
|
Indentation is wacky here. |
david
|
|
|
embedBox.hide() |
david
|
|
|
Please define a constant for the 1500. |
david
|
|
|
Trailing whitespace. |
david
|
|
|
This input should have the readonly property. |
david
|
|
|
This input should have the readonly property. |
david
|
|
|
Trailing whitespace. |
david
|
|
|
It looks like this file probably overlaps a lot with diff_file_fragment.html. Is it possible to abstract out the common parts? |
david
|
|
|
Please use <span> instead of <i> |
david
|
|
|
Trailing whitespace. |
david
|
|
|
Indentation within templates should be 1 space. |
david
|
|
|
Please mark this for translation ({% trans "Permalink" %}). |
david
|
|
|
Please mark for translation. |
david
|
|
|
Trailing whitespace. |
david
|
|
|
local variable 'diffset' is assigned to but never used |
reviewbot
|
|
|
local variable 'embed_links' is assigned to but never used |
reviewbot
|
|
|
Col: 80 E501 line too long (85 > 79 characters) |
reviewbot
|
|
|
Can we define this as its own patterns named diffviewer_embed_urls, and then include that from diffviewer_revision_urls? The it won't need … |
david
|
|
|
Javascript should be indented 4 spaces (python also 4, less is 2, templates are 1). Yeah, I'd love to just … |
david
|
|
|
If you're working on the latest master (with the pipeline changes that have gone in over the last few days), … |
david
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 9 E122 continuation line missing indentation or outdented |
reviewbot
|
|
|
Col: 13 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Trailing whitespace. |
brennie
|
|
|
Remove this line. |
brennie
|
|
|
Instead of returning false, this should take an event argument and do: e.preventDefault(). |
brennie
|
|
|
Indent with 4. |
brennie
|
|
|
/** Please indent this file with 4 spaces instead of 2. |
brennie
|
|
|
/** |
brennie
|
|
|
"Show and hide" |
brennie
|
|
|
How about toggleEmbedBox ? |
brennie
|
|
|
You don't need window for clearTimeout. |
brennie
|
|
|
Here this will also be the wrong thing. See below comment about _.bind vs ES6 fat arrow functions. |
brennie
|
|
|
This function will have this set to the $embedBox element. Therefore, this.FADE_DURATION and this.EMBED_BOX_SHORT_TIMER will be undefined. You'll want to … |
brennie
|
|
|
Fix indentation of the block statements. |
brennie
|
|
|
One less space here. |
brennie
|
|
|
Space between = and \ |
david
|
|
|
You don't need to pass in self when calling super methods. |
david
|
|
|
Would be simpler as: context['review_request'] = self.review_request |
david
|
|
|
This should probably be a separate change. |
david
|
|
|
Should be indented 2 more spaces. |
david
|
|
|
Indentation is kind of wacky here. |
david
|
|
|
Since you're using .es6.js, this can be: const $embedBox = ...; let longTimer; let shortTimer; |
david
|
|
|
This can be shorter: $embedBox.fadeOut(this.FADE_DURATION, () => $embedBox.hide()); |
david
|
|
|
This can be a bit shorter: $embedBox.fadeOut(this.FADE_DURATION, () => $embedBox.hide()); |
david
|
|
|
This needs to be indented an additional 4 spaces. |
david
|
|
|
Fat arrow function? |
david
|
|
|
You don't need the {} around the inner function. For wrapping purposes you could put () => ... on the … |
david
|
|
|
'login_required' imported but unused |
reviewbot
|
|
|
'augment_method_from' imported but unused |
reviewbot
|
|
|
Should include "Args" and "Returns" section. |
david
|
|
|
Tiny nit, but maybe swap the order of these so they match the order in the dict below? |
david
|
|
|
Please add a docstring. |
david
|
|
|
Please add Args/Returns. |
david
|
|
|
Remove this extra line. |
david
|
|
|
Please use "let" instead of "var" |
david
|
|
|
This can use a fat arrow function. |
david
|
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/urls.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/urls.py -
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/urls.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/urls.py
SA
Change Summary:
Added a new template file, updated the function call in the view to pass through some info about the review request/diff. Began to work on Backbone view/model.
Diff: |
Revision 4 (+155) |
|---|
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/templates/diffviewer/file_diff.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/templates/diffviewer/file_diff.html -
-
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/templates/diffviewer/file_diff.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/templates/diffviewer/file_diff.html
-
-
reviewboard/reviews/views.py (Diff revision 5) It might be worth examining how ReviewsDiffFragmentView works (as well as the base class DiffFragmentView). It looks like that thing takes a review_request_id, a revision, and a filediff_id, and spits out... well, diff fragments, which sounds like what you want. You might be able to re-use some machinery here.
-
-
reviewboard/reviews/views.py (Diff revision 5) Can you format this so that each parameter is not on its own line?
-
reviewboard/reviews/views.py (Diff revision 5) Docstrings should be of the form:
"""Single line summary. Multi-line description. """This is also missing an
ArgsandReturnsblock, e.g.:""" ... Args: request (django.http.HttpRequest): The HTTP request. review_request_id (type of review_request_id): Description of review_request_id.' ... Returns: django.http.HttpResponse: A description of the return value. """ -
reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js (Diff revision 5) We use the imperitive mood for doc strings and doc comment, so this should read as:
"Initialize the file diff embed page."
-
reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js (Diff revision 5) We use the imperitive mood for doc strings and doc comment, so this should read as:
"Initialize the file diff embed page."
-
-
-
reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js (Diff revision 5) render()shouldreturn this;. -
reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js (Diff revision 5) Put constants at the beginning
-
reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js (Diff revision 5) This doesn't do anything.
-
Just a couple very-high-level comments:
-
reviewboard/reviews/views.py (Diff revision 5) Function-based views are on their way out, and we should probably not write new ones. Can we make this a class-based view?
-
reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js (Diff revision 5) We can dispense with the whole router thing since users won't be able to change what revision they're looking at. This should simplify things quite a bit.
SA
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/templates/diffviewer/file_diff.html reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/templates/diffviewer/file_diff.html reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js
SA
Change Summary:
Updated summary.
Description: |
|
|---|
SA
Change Summary:
Added link to CodePen example.
Description: |
|
|---|
SA
Diff: |
Revision 7 (+238 -4)
|
|---|
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/pages/views/diffViewerPageView.js -
-
SA
Diff: |
Revision 8 (+487 -6)
|
|---|
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less -
-
-
-
-
-
SA
Diff: |
Revision 9 (+407 -7)
|
|---|
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less -
reviewboard/diffviewer/views.py (Diff revision 9) Col: 9 E122 continuation line missing indentation or outdented
-
reviewboard/diffviewer/views.py (Diff revision 9) Col: 13 E122 continuation line missing indentation or outdented
-
reviewboard/diffviewer/views.py (Diff revision 9) Col: 20 E251 unexpected spaces around keyword / parameter equals
-
reviewboard/diffviewer/views.py (Diff revision 9) Col: 19 E702 multiple statements on one line (semicolon)
-
reviewboard/diffviewer/views.py (Diff revision 9) local variable 'diffset' is assigned to but never used
-
reviewboard/diffviewer/views.py (Diff revision 9) Col: 9 E122 continuation line missing indentation or outdented
-
reviewboard/diffviewer/views.py (Diff revision 9) Col: 19 E702 multiple statements on one line (semicolon)
-
SA
Description: |
|
|---|
SA
Diff: |
Revision 10 (+401 -7)
|
|---|
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less
SA
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 11 (+684 -8)
|
-
File Captions:
Screen Shot 2016-02-14 at 5.29.12 PM.png: - Current state of embedded diff (note generated url) + Unstyled embedded diff (note generated url)
Added Files: |
|---|
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/pages/views/diffViewerPageView.js
-
This is a long review but it's mostly pretty simple stuff. I'm very happy with how this is going!
-
Can we maybe add some cute link back to the main diff that says something like "View this change on Review Board"?
-
-
reviewboard/diffviewer/views.py (Diff revision 11) These should all be sorted in alphabetical order.
-
-
-
reviewboard/diffviewer/views.py (Diff revision 11) This seems to share a lot with
DiffFragmentView. Can we create some intermediate parent class for both that includes the shared code? -
reviewboard/diffviewer/views.py (Diff revision 11) If there's no docstring, the first line can be immediately after the
class ...line. But maybe it's better to add a docstring. -
-
-
-
-
reviewboard/diffviewer/views.py (Diff revision 11) I know this is just copy/pasted, but we're trying to move towards using the imperative mood for docstrings as per PEP-257. Therefore this should be "Create the renderer" rather than "Creates the renderer"
-
reviewboard/diffviewer/views.py (Diff revision 11) Mind adding "Args:" and "Returns:" sections as per https://www.reviewboard.org/docs/codebase/dev/ ?
-
reviewboard/diffviewer/views.py (Diff revision 11) Mind adding "Args:" and "Returns:" sections as per https://www.reviewboard.org/docs/codebase/dev/ ?
-
-
reviewboard/diffviewer/views.py (Diff revision 11) Mind adding "Args:" and "Returns:" sections as per https://www.reviewboard.org/docs/codebase/dev/ ?
-
-
-
-
reviewboard/reviews/urls.py (Diff revision 11) Can you put this on the previous line?
name="diff-fragment-view"), -
-
-
-
-
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11) Can you make this alphabetical-ish?
-
-
-
-
reviewboard/static/rb/css/pages/reviews.less (Diff revision 11) Why this change? Something seems wrong.
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 11) I know fontawesome says to use
<i>but we generally use<span>instead. -
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 11) Can you make this one big
varstatement with individual variables separated by commas? -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 11) Since these are jquery objects, let's prefix the variable name with $
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 11) Space between
ifand(. We probably should also useembedBox.is(':visible')instead of checking the CSS value. -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 11) .css('display', 'block')is equivalent to.show() -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 11) This is the same as
embedBox.hide() -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 11) Please define a constant for the 5000.
-
-
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 11) We format these like:
embedBox .on('mouseenter' function() { // ... }) .on('mouseleave', function() { // ... }); -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 11) Space between
function()and{ -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 11) Indentation is wacky here.
-
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 11) Please define a constant for the 1500.
-
-
reviewboard/templates/diffviewer/diff_file_fragment.html (Diff revision 11) This input should have the
readonlyproperty. -
reviewboard/templates/diffviewer/diff_file_fragment.html (Diff revision 11) This input should have the
readonlyproperty. -
-
reviewboard/templates/diffviewer/diff_file_fragment_embed.html (Diff revision 11) It looks like this file probably overlaps a lot with diff_file_fragment.html. Is it possible to abstract out the common parts?
-
reviewboard/templates/diffviewer/diff_file_fragment_embed.html (Diff revision 11) Please use
<span>instead of<i> -
reviewboard/templates/diffviewer/diff_file_fragment_embed.html (Diff revision 11) Trailing whitespace.
-
reviewboard/templates/diffviewer/diff_file_fragment_embed.html (Diff revision 11) Indentation within templates should be 1 space.
-
reviewboard/templates/diffviewer/diff_file_fragment_embed.html (Diff revision 11) Please mark this for translation (
{% trans "Permalink" %}). -
reviewboard/templates/diffviewer/diff_file_fragment_embed.html (Diff revision 11) Please mark for translation.
-
reviewboard/templates/diffviewer/diff_file_fragment_embed.html (Diff revision 11) Trailing whitespace.
SA
Change Summary:
Fixes from David's review.
Diff: |
Revision 12 (+686 -6)
|
|---|
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/pages/views/diffViewerPageView.js -
reviewboard/diffviewer/views.py (Diff revision 12) local variable 'diffset' is assigned to but never used
-
reviewboard/diffviewer/views.py (Diff revision 12) local variable 'embed_links' is assigned to but never used
-
SA
Diff: |
Revision 13 (+686 -6)
|
|---|
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/pages/views/diffViewerPageView.js
SA
Diff: |
Revision 14 (+706 -7)
|
|---|
-
Tool: Pyflakes Processed Files: reviewboard/staticbundles.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/utils/fileDiffUtils.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/utils/fileDiffUtils.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js
-
-
reviewboard/reviews/urls.py (Diff revision 14) Can we define this as its own
patternsnameddiffviewer_embed_urls, and then include that fromdiffviewer_revision_urls? The it won't need the<revision>capture. -
reviewboard/static/rb/js/utils/fileDiffUtils.js (Diff revision 14) Javascript should be indented 4 spaces (python also 4, less is 2, templates are 1). Yeah, I'd love to just standardize everywhere :(
-
reviewboard/templates/diffviewer/diff_file_fragment_embed.html (Diff revision 14) If you're working on the latest master (with the pipeline changes that have gone in over the last few days), you should change this to load the
pipelinetags, and then changecompressed_csstostylesheetandcompressed_jstojavascript.
SA
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 15 (+609 -95)
|
-
Tool: Pyflakes Processed Files: reviewboard/staticbundles.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/utils/fileDiffUtils.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/utils/fileDiffUtils.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js
SA
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 16 (+630 -97)
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/staticbundles.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/utils/fileDiffUtils.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/staticbundles.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/utils/fileDiffUtils.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js -
SA
Diff: |
Revision 17 (+630 -97)
|
|---|
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/staticbundles.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/utils/fileDiffUtils.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/staticbundles.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/utils/fileDiffUtils.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js -
reviewboard/reviews/views.py (Diff revision 17) Col: 9 E122 continuation line missing indentation or outdented
SA
Diff: |
Revision 18 (+630 -97)
|
|---|
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/staticbundles.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/utils/fileDiffUtils.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/staticbundles.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/utils/fileDiffUtils.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js -
reviewboard/reviews/views.py (Diff revision 18) Col: 13 E128 continuation line under-indented for visual indent
SA
Diff: |
Revision 19 (+630 -97)
|
|---|
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/staticbundles.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/utils/fileDiffUtils.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/staticbundles.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/utils/fileDiffUtils.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js
-
-
-
-
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js (Diff revision 19) Instead of returning false, this should take an event argument and do:
e.preventDefault(). -
-
reviewboard/static/rb/js/utils/fileDiffUtils.js (Diff revision 19) /**Please indent this file with 4 spaces instead of 2.
-
-
-
-
reviewboard/static/rb/js/utils/fileDiffUtils.js (Diff revision 19) You don't need
windowforclearTimeout. -
reviewboard/static/rb/js/utils/fileDiffUtils.js (Diff revision 19) Here
thiswill also be the wrong thing. See below comment about_.bindvs ES6 fat arrow functions. -
reviewboard/static/rb/js/utils/fileDiffUtils.js (Diff revision 19) This function will have
thisset to the$embedBoxelement. Therefore,this.FADE_DURATIONandthis.EMBED_BOX_SHORT_TIMERwill beundefined.You'll want to use
_.bind()as such:$embedBox .on(/* ... */) .on('mouseleave', _.bind(function() { shortTimer = setTimeout(_.bind(function() { $embedBox.fadeOut(this.FADE_DURATION, function() { $embedBox.hide(); }); }, this), this.EMBED_BOX_SHORT_DURATION); }, this));Another option, however, is to use ES6 functionality for this. The latest
masterandrelease-2.6.xbranches support ES6 when JS files are named.es6.js. Then you could use:$embedBox .on(/* ... */) .on('mouseleave', () => { shortTimer = setTimeout(() => { $embedBox.fadeOut(this.FADE_DURATION, () => { $embedBox.hide(); }); }, this.EMBED_BOX_SHORT_DURATION); });which is much cleaner.
-
reviewboard/templates/diffviewer/diff_file_fragment_embed.html (Diff revision 19) Fix indentation of the block statements.
-
reviewboard/templates/diffviewer/diff_file_fragment_embed.html (Diff revision 19) One less space here.
SA
Diff: |
Revision 20 (+647 -97)
|
|---|
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/staticbundles.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/js/utils/fileDiffUtils.es6.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/staticbundles.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/js/utils/fileDiffUtils.es6.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/pages/views/diffViewerPageView.js
-
-
-
reviewboard/reviews/views.py (Diff revision 20) You don't need to pass in
selfwhen calling super methods. -
reviewboard/reviews/views.py (Diff revision 20) Would be simpler as:
context['review_request'] = self.review_request -
reviewboard/static/rb/css/pages/reviews.less (Diff revision 20) This should probably be a separate change.
-
reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js (Diff revision 20) Should be indented 2 more spaces.
-
reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js (Diff revision 20) Indentation is kind of wacky here.
-
reviewboard/static/rb/js/utils/fileDiffUtils.es6.js (Diff revision 20) Since you're using .es6.js, this can be:
const $embedBox = ...; let longTimer; let shortTimer; -
reviewboard/static/rb/js/utils/fileDiffUtils.es6.js (Diff revision 20) This can be shorter:
$embedBox.fadeOut(this.FADE_DURATION, () => $embedBox.hide()); -
reviewboard/static/rb/js/utils/fileDiffUtils.es6.js (Diff revision 20) This can be a bit shorter:
$embedBox.fadeOut(this.FADE_DURATION, () => $embedBox.hide()); -
reviewboard/static/rb/js/utils/fileDiffUtils.es6.js (Diff revision 20) This needs to be indented an additional 4 spaces.
-
-
reviewboard/static/rb/js/utils/fileDiffUtils.es6.js (Diff revision 20) You don't need the {} around the inner function. For wrapping purposes you could put
() => ...on the next line, aligned withthis.FADE_DURATION.
SA
Diff: |
Revision 21 (+649 -98)
|
|---|
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/staticbundles.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/js/utils/fileDiffUtils.es6.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/staticbundles.py reviewboard/diffviewer/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/static/rb/js/pages/views/fileDiffEmbedView.js reviewboard/templates/diffviewer/diff_file_fragment_embed.html reviewboard/static/rb/css/pages/diffviewer.less reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/js/utils/fileDiffUtils.es6.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/pages/views/diffViewerPageView.js -
-
-
Existing code is often using a mix of styles, but for new code, we'd like docstrings to be consistent. Please see https://www.reviewboard.org/docs/codebase/dev/docs/writing-codebase-docs/ for details on how things should be formatted.
-
-
reviewboard/diffviewer/views.py (Diff revision 21) Tiny nit, but maybe swap the order of these so they match the order in the dict below?
-
-
-
-
reviewboard/static/rb/js/utils/fileDiffUtils.es6.js (Diff revision 21) Please use "let" instead of "var"
-
reviewboard/static/rb/js/utils/fileDiffUtils.es6.js (Diff revision 21) This can use a fat arrow function.
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/urls.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/urls.py