Add infinite scrolling to the diff viewer
Review Request #7047 — Created March 12, 2015 and discarded
Add infinite scrolling to the diff viewer.
When working with long diffs, it's common for the diffs to be split up across
several pages. This can end up being a pain to review, as it means having to
deal with manual pagination.This change replaces the pagination to scroll loading. When user scroll to the
bottom of the page, diffs will be loaded automatically without having to click
anything.First bind a listening function to the window to check the position. And when
it detect scroll bar touch the bottom, it will active the loading function to
load next diffs. The loading function use ajax to load pagination's diffs, and
append to the current pagination. Similar to the set, when update the diffs,
properties should update one by one.Remove the pagination and page selection, and modify the description of 'paginate
by' and 'paginate orphans'.
Passed all js-tests.
Manual tests with a 25 diff files review request.
Paginate by 2, and paginate orphans is 10.When first load the pages, the page loads 2 diffs, and scroll to the bottom, it
loads 2 another difss. At last, the page loads 10 diffs.
| Description | From | Last Updated |
|---|---|---|
|
Col: 80 E501 line too long (80 > 79 characters) |
 reviewbot
|
|
|
I believe you can just remove the else part. |
SU Sunxperous | |
|
obejct -> object |
VT VTL-Developer | |
|
I suppose this should be ===? I think it will be clearer if ($(document).scrollTop() + $(window).height()) == ($(document).height()) is defined … |
SU Sunxperous | |
|
You can do this.isScrolling without the equality check. |
VT VTL-Developer | |
|
You don't actually use attrs anywhere in this function except to feature gate emptying the items table. If this is … |
 brennie
|
|
|
maybe try "Append another page object into the current page so that add(parse(...)) can't be called, due to part of … |
CR cristocrat | |
|
You're missing a space between that and add. How about "Append another set of diffs into the current page." |
brennie
|
|
|
No blank line here. |
brennie
|
|
|
Why are you checking this.attributes.property_name instead of attrs.property_name ? |
brennie
|
|
|
All var statements should be at the top of the function. |
brennie
|
|
|
Clarify what "has the next page" means. |
brennie
|
|
|
This seems to be private data that doesn't leave the DiffViewerPageView. This should be _isScrolling in that case. |
brennie
|
|
|
Can we rename this to _onScroll or _onWindowScroll or similar? |
brennie
|
|
|
maybe try "Listens to a scroll action and check whether it reaches the bottom of the page" |
CR cristocrat | |
|
This logic is kind of hard to grok. Can we assign the computed window top to a variable and then … |
brennie
|
|
|
This is a simplified statement so these should be fine on the same line. |
brennie
|
|
|
Only one var statement at the top of the function. |
brennie
|
|
|
Isn't the default behaviour to fetch the first page if the page query parameter isn't provided? If that is the … |
brennie
|
|
|
Won't there be no more references to the XHR object anyway? Shouldn't it get automatically garbage collected? |
brennie
|
|
|
How about "Load the next page of diff content" ? |
brennie
|
|
|
maybe try "Use AJAX to load the next pagination page and append..." |
CR cristocrat | |
|
Again, why won't the GC kick in to get rid of the XHR object? |
brennie
|
|
|
We usually do if (positive_case) { // ... } else { // ... } instead of ```javascript if (!positive_case) { … |
brennie
|
|
|
See my previous comment about XHR objects getting GC'ed. |
brennie
|
|
|
How about "before loading the diff viewer another time." ? |
brennie
|
|
|
Remove trailing whitespace. |
brennie
|
|
|
Blank line between these. |
brennie
|
|
|
Semicolon, not comma. |
brennie
|
|
|
Semicolon, not comma. |
brennie
|
|
|
We're not actually setting it to null anymore so can we remove this line? |
brennie
|
|
|
Doesn't look like this is used. |
 chipx86
|
|
|
Wrapping window and document is expensive. We should do that once, in render, storing as this._$window and this._$document, so it'll … |
chipx86
|
|
|
No need for the parens around the comparison. |
chipx86
|
|
|
You can do if (!attrs) |
chipx86
|
|
|
Similar to Christian's comment earlier, (!attrs)? |
SU Sunxperous | |
|
Add a blank line between the two lines, multi-line documentation should have a summary, followed by a blank line, then … |
SU Sunxperous | |
|
There's an extra *. |
SU Sunxperous | |
|
Not very sure here, but maybe this can be on the same line. |
SU Sunxperous | |
|
Why not just assign $diffs = $('#diffs'); in the beginning, and then only call $diffs.empty(); in the if block? |
SU Sunxperous | |
|
How about nextPage instead of nextpage? |
SU Sunxperous |
MI
- Change Summary:
-
Fix the bugs and new bug
- Commit:
-
56e8fe47e4f6a783145c0d6c7d30e607e64fdeba64999804e50dc7b4dc86a82489ae1da278988a47
-
Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
MI
- Change Summary:
-
Rebase
- Commit:
-
64999804e50dc7b4dc86a82489ae1da278988a47e14bc00ce4893974fba9bffd2f5234ad28aa6f31
-
Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
MI
- Change Summary:
-
Fix load Revision bugs
- Commit:
-
e14bc00ce4893974fba9bffd2f5234ad28aa6f316cccb05a69c3be3dca284efc75aa13867189b55e
-
Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
MI
- Change Summary:
-
Modify description, rebase from master, cut down part of the memory use.
- Commit:
-
6cccb05a69c3be3dca284efc75aa13867189b55e161290fcc75c70790998d8f8d27b4f257dd84b3a
-
Tool: Pyflakes Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js -
MI
- Commit:
-
161290fcc75c70790998d8f8d27b4f257dd84b3a4a3d0c11eb4c71943ea3107644e3fa917d6343f2
-
Tool: Pyflakes Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
MI
- Change Summary:
-
Change the summary, description, testing.
- Summary:
-
[WIP] Add infinite scrolling to the diff viewerAdd infinite scrolling to the diff viewer
- Description:
-
~ [WIP] Scroll Down Automatically Load Next Page
~ Add infinite scrolling to the diff viewer.
+ + When working with long diffs, it's common for the diffs to be split up across
+ several pages. This can end up being a pain to review, as it means having to + deal with manual pagination. + + This change replaces the pagination to scroll loading. When user scroll to the
+ bottom of the page, diffs will be loaded automatically without having to click + anything. + + First bind a listening function to the window to check the position. And when
+ it detect scroll bar touch the bottom, it will active the loading function to + load next diffs. The loading function use ajax to load pagination's diffs, and + append to the current pagination. Similar to the set, when update the diffs, + properties should update one by one. + + Remove the pagination and page selection, and modify the description of 'paginate
+ by' and 'paginate orphans'. - Testing Done:
-
~ No Test Now.
~ Passed all js-tests.
+ + Manual tests with a 25 diff files review request.
+ Paginate by 2, and paginate orphans is 10. + + When first load the pages, the page loads 2 diffs, and scroll to the bottom, it
+ loads 2 another difss. At last, the page loads 10 diffs.
-
Tool: PEP8 Style Checker Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js Tool: Pyflakes Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
-
Tool: PEP8 Style Checker Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js Tool: Pyflakes Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
-
Tool: Pyflakes Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
-
-
You don't actually use
attrsanywhere in this function except to feature gate emptying the items table.If this is a flag, we should be explicit and call it something like
dontEmptyTableand explicitly check if it is nottrue. -
-
-
-
-
This seems to be private data that doesn't leave the
DiffViewerPageView. This should be_isScrollingin that case. -
-
This logic is kind of hard to grok.
Can we assign the computed window top to a variable and then do the comparison? Also when we break up an if statment we try to have each condition on a single line, e.g.:
if (really_long_condition_one && really_long_condition_two) { // ... } -
-
Isn't the default behaviour to fetch the first page if the
pagequery parameter isn't provided? If that is the case, we don't need to append&page=1to the URL all the time. -
Won't there be no more references to the XHR object anyway? Shouldn't it get automatically garbage collected?
-
-
-
We usually do
if (positive_case) { // ... } else { // ... }instead of
```javascript
if (!positive_case) {
// ...
} else {
// ...
} -
MI
- Change Summary:
-
Fix issues
- Commit:
-
d9a8404e2dd0d761d01842d824a7bdc8403dcafd0189c5b732259b7844599f08c147e1f2b012cdaa
-
Tool: Pyflakes Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
MI
- Change Summary:
-
Fix revision 10 issues
- Commit:
-
0189c5b732259b7844599f08c147e1f2b012cdaa27112153efc69683bc603e2591487c94a8d0e19c
-
Tool: PEP8 Style Checker Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js Tool: Pyflakes Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
-
Tool: Pyflakes Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
MI
- Change Summary:
-
Fix Revision 12 Issues
- Commit:
-
5fe724d72fc5f472f165b7ed53c422d1916733a62a2a19ac4733c26e5267a39e04e5ba2d09829569
-
Tool: PEP8 Style Checker Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js Tool: Pyflakes Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
-
Tool: PEP8 Style Checker Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js Tool: Pyflakes Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
-
-
-
Wrapping
windowanddocumentis expensive. We should do that once, inrender, storing asthis._$windowandthis._$document, so it'll be faster to handle scrolls.Another way to improve performance is to listen for
resizeevents and cache the window height and document height there, so tha scrolling doesn't need to recompute each time. That would make this function very fast. -
-
MI
- Change Summary:
-
Fix revision 14 issues
- Commit:
-
2a2a19ac4733c26e5267a39e04e5ba2d0982956903542e057beb31d9adb7473804d6e148f889d1da
-
Tool: Pyflakes Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
MI
- Change Summary:
-
Rebase from master
- Commit:
-
03542e057beb31d9adb7473804d6e148f889d1da325a4599d82d8ba6106f0656ac3496a9b8621c5b
-
Tool: Pyflakes Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
MI
- Change Summary:
-
Fix revision 16 issues.
- Commit:
-
325a4599d82d8ba6106f0656ac3496a9b8621c5b72968191c6c7b9330a6786c635ea4be3e2fd26e6
-
Tool: Pyflakes Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/admin/forms.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/js/pages/models/diffViewerPageModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/pages/models/diffViewerPageModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js