Display horizontal bar which indicates peer's scroll position.

Review Request #6194 — Created Aug. 2, 2014 and submitted

Information

rb-extension-pack
0b56533...

Reviewers

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 …

anselinaanselina

There's some trailing whitespace here.

anselinaanselina

There's some trailing whitespace here and below.

anselinaanselina

Isn't each peer given a unique colour that we use instead? Why do we need to set this?

mike_conleymike_conley

This needs var in front of it... although, why was this moved out of the initialize function?

mike_conleymike_conley

You can just coerce the length into a boolean, since 0 is falsey: if (!scroll_position_bar.length) { // Do things }

mike_conleymike_conley

Try: $('<div/>') .attr("id", "test") .addClass("togetherjs-peer-scroll-position") .css("background-color", peer_color) .appendTo(document.body);

mike_conleymike_conley

What's the 25 magic number for?

mike_conleymike_conley

Where did 50 get chosen and why? Also, as a general rule, when using parseInt, tell JS that you're parsing …

mike_conleymike_conley

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

mike_conleymike_conley

Col: 5 E128 continuation line under-indented for visual indent

reviewbotreviewbot
reviewbot
  1. 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
    
    
  2. 
      
PE
anselina
  1. 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.).

  2. review_together/README.md (Diff revision 1)
     
     
    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.

  3. review_together/README.md (Diff revision 1)
     
     
    Show all issues

    There's some trailing whitespace here.

  4. Show all issues

    There's some trailing whitespace here and below.

  5. 
      
david
  1. 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.

    1. Hi David,

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

  2. 
      
PE
PE
reviewbot
  1. 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
    
    
  2. 
      
david
  1. Your diff here seems to contain unrelated changes. Please update again and make sure to specify the exact range of commits.

  2. 
      
PE
reviewbot
  1. 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
    
    
  2. 
      
PE
reviewbot
  1. 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
    
    
  2. 
      
mike_conley
  1. 
      
  2. Show all issues

    Isn't each peer given a unique colour that we use instead? Why do we need to set this?

  3. Show all issues

    This needs var in front of it... although, why was this moved out of the initialize function?

  4. Show all issues

    You can just coerce the length into a boolean, since 0 is falsey:

    if (!scroll_position_bar.length) {
    // Do things
    }

  5. review_together/review_together/static/js/together.js (Diff revision 4)
     
     
     
     
     
     
    Show all issues

    Try:

    $('<div/>')
    .attr("id", "test")
    .addClass("togetherjs-peer-scroll-position")
    .css("background-color", peer_color)
    .appendTo(document.body);

  6. Show all issues

    What's the 25 magic number for?

  7. 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)

  8. Show all issues

    If we go with this, the equivalent is:

    scroll_position_bar.toggle(parseInt(peer_transmittion.position, 10) >= 50);

  9. 
      
PE
reviewbot
  1. 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
    
    
  2. 
      
PE
reviewbot
  1. 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
    
    
  2. 
      
PE
PE
reviewbot
  1. 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
    
    
  2. Show all issues
    Col: 5
     E128 continuation line under-indented for visual indent
    
  3. 
      
PE
Review request changed
Status:
Completed
Change Summary:
Pushed to ucosp/PeterTran/review-together-scroll-position (e29f940)