Display horizontal bar which indicates peer's scroll position.
Review Request #6194 — Created Aug. 2, 2014 and submitted
| Information | |
|---|---|
| PeterTran | |
| rb-extension-pack | |
|
|
|
| 0b56533... | |
| Reviewers | |
| reviewboard | |
Display horizontal bar which indicates peer's scroll position. [WIP]
Launched TogetherJS in one browser. Opened a second browser and entered the URL created by the first browser. Scrolled around on both browsers to check if horizontal bar displayed and moved with the other browser.
Demostration Video:
http://youtu.be/OO7Qc_9jiz8
| Description | From | Last Updated |
|---|---|---|
|
I know this is a typo from before you worked on this, but can you change this to python setup.py … |
 anselina
|
|
|
There's some trailing whitespace here. |
anselina
|
|
|
There's some trailing whitespace here and below. |
anselina
|
|
|
Isn't each peer given a unique colour that we use instead? Why do we need to set this? |
 mike_conley
|
|
|
This needs var in front of it... although, why was this moved out of the initialize function? |
mike_conley
|
|
|
You can just coerce the length into a boolean, since 0 is falsey: if (!scroll_position_bar.length) { // Do things } |
mike_conley
|
|
|
Try: $('<div/>') .attr("id", "test") .addClass("togetherjs-peer-scroll-position") .css("background-color", peer_color) .appendTo(document.body); |
mike_conley
|
|
|
What's the 25 magic number for? |
mike_conley
|
|
|
Where did 50 get chosen and why? Also, as a general rule, when using parseInt, tell JS that you're parsing … |
mike_conley
|
|
|
If we go with this, the equivalent is: scroll_position_bar.toggle(parseInt(peer_transmittion.position, 10) >= 50); |
mike_conley
|
|
|
Col: 5 E128 continuation line under-indented for visual indent |
 reviewbot
|
-
This is a very cursory review - I just wanted to point out the installation instruction fix before forgetting it!
Also, it looks like your changes from /r/6102 and /r/6066 are also in this review request. Could you fix it so only the relevant changes are in this one? Same thing applies for /r/6102 (changes to review-together.less should not be in there, etc.).
-
review_together/README.md (Diff revision 1) I know this is a typo from before you worked on this, but can you change this to
python setup.py install? The installation won't work otherwise. -
-
review_together/review_together/static/js/together.js (Diff revision 1) There's some trailing whitespace here and below.
-
Just a reminder, you had said you were going to show a video and/or screenshot of this functionality for us to look at. Time is running low, so we'd like to see that soon to give you feedback on it.
PE
Testing Done: |
|
|---|
PE
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+1055 -23) |
-
Tool: PEP8 Style Checker Processed Files: review_together/review_together/forms.py review_together/review_together/admin_urls.py review_together/review_together/extension.py Ignored Files: review_together/review_together/static/js/togetherjs-min.js review_together/README.md review_together/review_together/static/js/together.js review_together/review_together/static/css/review-together.less review_together/review_together/templates/review_together/base.html Tool: Pyflakes Processed Files: review_together/review_together/forms.py review_together/review_together/admin_urls.py review_together/review_together/extension.py Ignored Files: review_together/review_together/static/js/togetherjs-min.js review_together/README.md review_together/review_together/static/js/together.js review_together/review_together/static/css/review-together.less review_together/review_together/templates/review_together/base.html
-
Your diff here seems to contain unrelated changes. Please update again and make sure to specify the exact range of commits.
-
Tool: PEP8 Style Checker Ignored Files: review_together/README.md review_together/review_together/static/js/together.js Tool: Pyflakes Ignored Files: review_together/README.md review_together/review_together/static/js/together.js
-
Tool: Pyflakes Ignored Files: review_together/README.md review_together/review_together/static/js/together.js review_together/review_together/static/css/review-together.less Tool: PEP8 Style Checker Ignored Files: review_together/README.md review_together/review_together/static/js/together.js review_together/review_together/static/css/review-together.less
-
-
review_together/review_together/static/css/review-together.less (Diff revision 4) Isn't each peer given a unique colour that we use instead? Why do we need to set this?
-
review_together/review_together/static/js/together.js (Diff revision 4) This needs var in front of it... although, why was this moved out of the initialize function?
-
review_together/review_together/static/js/together.js (Diff revision 4) You can just coerce the length into a boolean, since 0 is falsey:
if (!scroll_position_bar.length) {
// Do things
} -
review_together/review_together/static/js/together.js (Diff revision 4) Try:
$('<div/>')
.attr("id", "test")
.addClass("togetherjs-peer-scroll-position")
.css("background-color", peer_color)
.appendTo(document.body); -
review_together/review_together/static/js/together.js (Diff revision 4) What's the 25 magic number for?
-
review_together/review_together/static/js/together.js (Diff revision 4) Where did 50 get chosen and why?
Also, as a general rule, when using parseInt, tell JS that you're parsing in base 10.
Like this:
parseInt(peer_transmission.position, 10)
-
review_together/review_together/static/js/together.js (Diff revision 4) If we go with this, the equivalent is:
scroll_position_bar.toggle(parseInt(peer_transmittion.position, 10) >= 50);
PE
-
Tool: Pyflakes Processed Files: review_together/review_together/forms.py review_together/review_together/admin_urls.py review_together/review_together/extension.py Ignored Files: review_together/review_together/static/js/togetherjs-min.js review_together/README.md review_together/review_together/static/js/together.js review_together/review_together/static/css/review-together.less review_together/review_together/templates/review_together/base.html Tool: PEP8 Style Checker Processed Files: review_together/review_together/forms.py review_together/review_together/admin_urls.py review_together/review_together/extension.py Ignored Files: review_together/review_together/static/js/togetherjs-min.js review_together/README.md review_together/review_together/static/js/together.js review_together/review_together/static/css/review-together.less review_together/review_together/templates/review_together/base.html
PE
Diff: |
Revision 6 (+97 -32)
|
|---|
-
Tool: Pyflakes Processed Files: review_together/review_together/forms.py review_together/review_together/admin_urls.py review_together/review_together/extension.py Ignored Files: review_together/review_together/static/js/review-together.js review_together/README.md review_together/review_together/static/js/together.js review_together/review_together/static/css/review-together.less review_together/review_together/static/js/togetherjs-min.js Tool: PEP8 Style Checker Processed Files: review_together/review_together/forms.py review_together/review_together/admin_urls.py review_together/review_together/extension.py Ignored Files: review_together/review_together/static/js/review-together.js review_together/README.md review_together/review_together/static/js/together.js review_together/review_together/static/css/review-together.less review_together/review_together/static/js/togetherjs-min.js
PE
Commit: |
|
||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+953 -20)
|
-
Tool: PEP8 Style Checker Processed Files: review_together/review_together/forms.py review_together/review_together/admin_urls.py review_together/review_together/extension.py Ignored Files: review_together/README.md review_together/review_together/static/css/review-together.less review_together/review_together/static/js/togetherjs-min.js review_together/review_together/static/js/review-together.js review_together/review_together/static/js/together.js review_together/review_together/templates/review_together/base.html Tool: Pyflakes Processed Files: review_together/review_together/forms.py review_together/review_together/admin_urls.py review_together/review_together/extension.py Ignored Files: review_together/README.md review_together/review_together/static/css/review-together.less review_together/review_together/static/js/togetherjs-min.js review_together/review_together/static/js/review-together.js review_together/review_together/static/js/together.js review_together/review_together/templates/review_together/base.html -
review_together/review_together/admin_urls.py (Diff revision 7) Col: 5 E128 continuation line under-indented for visual indent
Tool: PEP8 Style Checker Processed Files: review_together/review_together/forms.py review_together/review_together/admin_urls.py review_together/review_together/extension.py Ignored Files: review_together/review_together/static/js/togetherjs-min.js review_together/README.md review_together/review_together/static/js/together.js review_together/review_together/static/css/review-together.less review_together/review_together/templates/review_together/base.html Tool: Pyflakes Processed Files: review_together/review_together/forms.py review_together/review_together/admin_urls.py review_together/review_together/extension.py Ignored Files: review_together/review_together/static/js/togetherjs-min.js review_together/README.md review_together/review_together/static/js/together.js review_together/review_together/static/css/review-together.less review_together/review_together/templates/review_together/base.html