Summary

Display horizontal bar which indicates peer's scroll position.

Review Request #6194 — Created Aug. 2, 2014 and submitted Jan. 8, 2015, 4:17 p.m.

Information

Owner

PeterTran

Repository

rb-extension-pack

Branch

Bugs

Depends On

~~6102~~

Commit

0b56533...

Reviewers

Groups

reviewboard

People

Description

Display horizontal bar which indicates peer's scroll position. [WIP]

Testing Done

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

Issues

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	Aug. 7, 2014, 5:15 p.m.
There's some trailing whitespace here.	anselina	Aug. 7, 2014, 5:15 p.m.
There's some trailing whitespace here and below.	anselina	Aug. 7, 2014, 5:16 p.m.
Isn't each peer given a unique colour that we use instead? Why do we need to set this?	mike_conley	Aug. 12, 2014, 12:43 p.m.
This needs var in front of it... although, why was this moved out of the initialize function?	mike_conley	Aug. 12, 2014, 12:44 p.m.
You can just coerce the length into a boolean, since 0 is falsey: if (!scroll_position_bar.length) { // Do things }	mike_conley	Aug. 12, 2014, 12:45 p.m.
Try: $('<div/>') .attr("id", "test") .addClass("togetherjs-peer-scroll-position") .css("background-color", peer_color) .appendTo(document.body);	mike_conley	Aug. 12, 2014, 12:47 p.m.
What's the 25 magic number for?	mike_conley	Aug. 12, 2014, 12:51 p.m.
Where did 50 get chosen and why? Also, as a general rule, when using parseInt, tell JS that you're parsing …	mike_conley	Aug. 12, 2014, 12:52 p.m.
If we go with this, the equivalent is: scroll_position_bar.toggle(parseInt(peer_transmittion.position, 10) >= 50);	mike_conley	Aug. 12, 2014, 12:52 p.m.
Col: 5 E128 continuation line under-indented for visual indent	reviewbot	Jan. 8, 2015, 4:17 p.m.

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

Description:: ~
Display horizontal bar which indicates peer's scroll position.
~
Display horizontal bar which indicates peer's scroll position. [WIP]

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)

The issue has been resolved. Show all issues

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/README.md (Diff revision 1)
The issue has been resolved. Show all issues
```
There's some trailing whitespace here.
```
review_together/review_together/static/js/together.js (Diff revision 1)
The issue has been resolved. Show all issues
```
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.

PeterTran Aug. 7, 2014, 5:17 p.m.

Hi David,

Sorry for the delay. I just uploaded a video on youtube:
http://youtu.be/OO7Qc_9jiz8

Testing Done:: 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

Commit:

d6ba6ee27a0f22ed805035d34347f5093956739d

1c7b1a3e5ada378f912279553de0c34fdb064d51

Diff:

Revision 2 (+1055 -23)

Show changes

	review_together/README.md
	review_together/review_together/admin_urls.py
	review_together/review_together/extension.py
	review_together/review_together/forms.py
	review_together/review_together/static/css/review-together.less
	review_together/review_together/static/js/together.js
	review_together/review_together/static/js/togetherjs-min.js
	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



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.

Commit:

1c7b1a3e5ada378f912279553de0c34fdb064d51

Diff:

Revision 3 (+6 -7)

Show changes

	review_together/README.md
	review_together/review_together/static/js/together.js

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

Diff:

Revision 4 (+77 -12)

Show changes

	review_together/README.md
	review_together/review_together/static/css/review-together.less
	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)
The issue has been resolved. Show all issues
```
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)
The issue has been resolved. Show all issues
```
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)

The issue has been resolved. Show all issues

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)

The issue has been resolved. Show all issues

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)
The issue has been resolved. Show all issues
```
What's the 25 magic number for?
```

review_together/review_together/static/js/together.js (Diff revision 4)

The issue has been resolved. Show all issues

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)

The issue has been resolved. Show all issues

If we go with this, the equivalent is:
scroll_position_bar.toggle(parseInt(peer_transmittion.position, 10) >= 50);

Commit:

26e4f5db8de736f8a4490beaf9487e950ffaa2f2

Diff:

Revision 5 (+1055 -23)

Show changes

	review_together/README.md
	review_together/review_together/admin_urls.py
	review_together/review_together/extension.py
	review_together/review_together/forms.py
	review_together/review_together/static/css/review-together.less
	review_together/review_together/static/js/together.js
	review_together/review_together/static/js/togetherjs-min.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/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

Diff:

Revision 6 (+97 -32)

Show changes

	review_together/README.md
	review_together/review_together/admin_urls.py
	review_together/review_together/extension.py
	review_together/review_together/forms.py
	review_together/review_together/static/css/review-together.less
	review_together/review_together/static/js/review-together.js
	review_together/review_together/static/js/together.js
	review_together/review_together/static/js/togetherjs.js

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

Depends On:: ~~6102 - User is able to specify their own hub in extension configurations.~~

Commit:

26e4f5db8de736f8a4490beaf9487e950ffaa2f2

0b56533c20725f146a13d1f7bf34e80352f92d31

Diff:

Revision 7 (+953 -20)

Show changes

	review_together/README.md
	review_together/review_together/admin_urls.py
	review_together/review_together/extension.py
	review_together/review_together/forms.py
	review_together/review_together/static/css/review-together.less
	review_together/review_together/static/js/review-together.js
	review_together/review_together/static/js/together.js
	review_together/review_together/static/js/togetherjs-min.js
	1 more

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)
The issue has been resolved. Show all issues
```
Col: 5
 E128 continuation line under-indented for visual indent
```

Status:: Completed
Change Summary:: Pushed to ucosp/PeterTran/review-together-scroll-position (e29f940)

		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