[WIP] Add ability to embed per file diffs on external sites
Review Request #7890 — Created Jan. 17, 2016 and discarded
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
- 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.
-
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
-
-
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.
-
-
-
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. """ -
We use the imperitive mood for doc strings and doc comment, so this should read as:
"Initialize the file diff embed page."
-
We use the imperitive mood for doc strings and doc comment, so this should read as:
"Initialize the file diff embed page."
-
-
-
-
-
-
Just a couple very-high-level comments:
-
Function-based views are on their way out, and we should probably not write new ones. Can we make this a class-based view?
-
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.
- Diff:
-
Revision 6 (+258 -3)
-
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
- Change Summary:
-
Updated summary.
- Description:
-
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 small 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 plan is to have it return the
necessary files to display a single FileDiff – 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.
- Testing Done:
-
- Tested URI in browser with no ill side effects.
+ - Embed box needs to be fixed in Safari. Timeout is too fast.
- Change Summary:
-
Added link to CodePen example.
- Description:
-
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 small 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 plan is to have it return the
necessary files to display a single FileDiff – 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. + +
- 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 -
-
- 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 -
-
-
-
-
-
- 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 -
-
-
-
-
-
-
-
- Description:
-
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 small 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 plan is to have it return the
necessary files to display a single FileDiff – 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. + + In the most recent update to this feature, the full URL
+ is generated and placed in the diff viewer dropdown box + (however, only in the embedded diff). Previously, the path + was only included and now the full URL, with the HTTP host, + is included. + + The same functionality should work on the regular diff viewer, as
+ it now includes the same components to generate the URL and place + it in the template, however it is currently not displaying.
- 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
- Description:
-
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 small 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 plan is to have it return the
necessary files to display a single FileDiff – 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. ~ In the most recent update to this feature, the full URL
~ The full URL is generated and placed in the diff viewer dropdown box
- is generated and placed in the diff viewer dropdown box (however, only in the embedded diff). Previously, the path was only included and now the full URL, with the HTTP host, is included. The same functionality should work on the regular diff viewer, as
it now includes the same components to generate the URL and place it in the template, however it is currently not displaying. + + 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. - 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"?
-
-
-
-
-
This seems to share a lot with
DiffFragmentView. Can we create some intermediate parent class for both that includes the shared code? -
If there's no docstring, the first line can be immediately after the
class ...line. But maybe it's better to add a docstring. -
-
-
-
-
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"
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
Space between
ifand(. We probably should also useembedBox.is(':visible')instead of checking the CSS value. -
-
-
-
-
-
We format these like:
embedBox .on('mouseenter' function() { // ... }) .on('mouseleave', function() { // ... }); -
-
-
-
-
-
-
-
-
It looks like this file probably overlaps a lot with diff_file_fragment.html. Is it possible to abstract out the common parts?
-
-
-
-
-
-
- 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 -
-
-
- 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
- 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
-
-
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. -
Javascript should be indented 4 spaces (python also 4, less is 2, templates are 1). Yeah, I'd love to just standardize everywhere :(
-
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.
- Description:
-
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 small 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 plan is to have it return the
necessary files to display a single FileDiff – 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
(however, only in the embedded diff). Previously, the path was only included and now the full URL, with the HTTP host, is included. The same functionality should work on the regular diff viewer, as
it now includes the same components to generate the URL and place it in the template, however it is currently not displaying. 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. - 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
- Description:
-
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 small change creates an endpoint for the individual FileDiffs
~ 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 plan is to have it return the
~ necessary files to display a single FileDiff – as if it were ~ When this endpoint is hit, the necessary files to display a single
~ Diff fragment are returned as if it were extracted from the DiffViewer. - 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
~ (however, only in the embedded diff). Previously, the path ~ was only included and now the full URL, with the HTTP host, ~ Previously, the path was only included and now the full URL, with ~ the HTTP host, is included. - is included. - - The same functionality should work on the regular diff viewer, as
- it now includes the same components to generate the URL and place - it in the template, however it is currently not displaying. 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. - 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 -
- 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 -
- 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 -
- 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
-
-
-
-
-
-
-
-
-
-
-
-
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.
-
-
- 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
- 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 -
-
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