Created extension to allow GIF webcam uploads with comments

Review Request #9540 — Created Jan. 26, 2018 and updated

rpolyano
rb-extension-pack
master
92d1891...
rb-extension-pack, students

This extension will allow users to capture short (3-5 second) videos of themselves, which are then encoded as animated GIFs client-side, and then uploaded and embedded into a comment on a code review.

The extension is now fully implemented for pop-over comments.

The gif.js library and worker (which are excluded from review) can be found here: https://github.com/jnordberg/gif.js/tree/master/dist
They should be placed in \static\js\lib\gif.js and \static\js\lib\gif.worker.js

Test comments were posted on dummy reviews
Tried canceling out of recording in both preview and recording states by clicking cancel, unchecking and rechecking Enable Markdown
Tested extension functions in Chrome, Edge, and Firefox

Description From Last Updated

Can you edit the description to explain where to get js/lib/gif.js ?

daviddavid

E302 expected 2 blank lines, found 1

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

E261 at least two spaces before inline comment

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

Col: 60 Missing semicolon.

reviewbotreviewbot

Col: 31 Expected '{' and instead saw 'return'.

reviewbotreviewbot

Col: 32 Expected '{' and instead saw 'return'.

reviewbotreviewbot

Col: 32 Expected '{' and instead saw 'console'.

reviewbotreviewbot

Col: 88 Missing semicolon.

reviewbotreviewbot

Col: 91 Missing semicolon.

reviewbotreviewbot

Col: 152 Missing semicolon.

reviewbotreviewbot

Col: 204 Missing semicolon.

reviewbotreviewbot

Col: 250 Missing semicolon.

reviewbotreviewbot

Col: 292 Missing semicolon.

reviewbotreviewbot

Col: 304 Missing semicolon.

reviewbotreviewbot

Col: 438 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 560 Missing semicolon.

reviewbotreviewbot

Col: 314 Missing semicolon.

reviewbotreviewbot

Col: 467 Expected '{' and instead saw 'return'.

reviewbotreviewbot

Col: 487 Expected '{' and instead saw 'return'.

reviewbotreviewbot

Col: 560 Expected an identifier and instead saw '='.

reviewbotreviewbot

Col: 560 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 561 Missing semicolon.

reviewbotreviewbot

Col: 580 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 581 Missing semicolon.

reviewbotreviewbot

Col: 673 Missing semicolon.

reviewbotreviewbot

Col: 695 Missing semicolon.

reviewbotreviewbot

Col: 715 Missing semicolon.

reviewbotreviewbot

Col: 738 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 720 'i' was used before it was defined.

reviewbotreviewbot

Col: 785 Expected '{' and instead saw 's'.

reviewbotreviewbot

Col: 801 Missing semicolon.

reviewbotreviewbot

Col: 942 Missing semicolon.

reviewbotreviewbot

Col: 1216 Expected '{' and instead saw 'throw'.

reviewbotreviewbot

E265 block comment should start with '# '

reviewbotreviewbot

E501 line too long (90 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 65 Missing semicolon.

reviewbotreviewbot

Col: 13 '$previewImage' is defined but never used.

reviewbotreviewbot

Col: 13 '$previewVideo' is defined but never used.

reviewbotreviewbot

Col: 13 '$startButton' is defined but never used.

reviewbotreviewbot

Col: 13 '$stopButton' is defined but never used.

reviewbotreviewbot

Col: 13 '$saveButton' is defined but never used.

reviewbotreviewbot

Col: 9 Do not use 'new' for side effects.

reviewbotreviewbot

Col: 5 'GifRecorder' is defined but never used.

reviewbotreviewbot

Col: 60 Missing semicolon.

reviewbotreviewbot

Col: 31 Expected '{' and instead saw 'return'.

reviewbotreviewbot

Col: 32 Expected '{' and instead saw 'return'.

reviewbotreviewbot

Col: 5 'GifEncoderAbstraction' was used before it was defined.

reviewbotreviewbot

Col: 32 Expected '{' and instead saw 'console'.

reviewbotreviewbot

Col: 88 Missing semicolon.

reviewbotreviewbot

Col: 91 Missing semicolon.

reviewbotreviewbot

Col: 152 Missing semicolon.

reviewbotreviewbot

Col: 204 Missing semicolon.

reviewbotreviewbot

Col: 250 Missing semicolon.

reviewbotreviewbot

Col: 292 Missing semicolon.

reviewbotreviewbot

Col: 304 Missing semicolon.

reviewbotreviewbot

Col: 314 Missing semicolon.

reviewbotreviewbot

Col: 438 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 467 Expected '{' and instead saw 'return'.

reviewbotreviewbot

Col: 560 Missing semicolon.

reviewbotreviewbot

Col: 560 Expected an identifier and instead saw '='.

reviewbotreviewbot

Col: 561 Missing semicolon.

reviewbotreviewbot

Col: 581 Missing semicolon.

reviewbotreviewbot

Col: 487 Expected '{' and instead saw 'return'.

reviewbotreviewbot

Col: 560 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 580 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

E501 line too long (90 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 9 Do not use 'new' for side effects.

reviewbotreviewbot

Col: 7 'GifRecorder' is defined but never used.

reviewbotreviewbot

Col: 60 Missing semicolon.

reviewbotreviewbot

Col: 60 Missing semicolon.

reviewbotreviewbot

Col: 31 Expected '{' and instead saw 'return'.

reviewbotreviewbot

Col: 32 Expected '{' and instead saw 'return'.

reviewbotreviewbot

Col: 46 Missing semicolon.

reviewbotreviewbot

Col: 5 'GifEncoderAbstraction' was used before it was defined.

reviewbotreviewbot

Col: 32 Expected '{' and instead saw 'console'.

reviewbotreviewbot

Col: 88 Missing semicolon.

reviewbotreviewbot

Col: 91 Missing semicolon.

reviewbotreviewbot

Col: 152 Missing semicolon.

reviewbotreviewbot

Col: 204 Missing semicolon.

reviewbotreviewbot

Col: 250 Missing semicolon.

reviewbotreviewbot

Col: 292 Missing semicolon.

reviewbotreviewbot

Col: 304 Missing semicolon.

reviewbotreviewbot

Col: 314 Missing semicolon.

reviewbotreviewbot

Col: 438 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 467 Expected '{' and instead saw 'return'.

reviewbotreviewbot

Col: 487 Expected '{' and instead saw 'return'.

reviewbotreviewbot

Col: 560 Missing semicolon.

reviewbotreviewbot

Col: 561 Missing semicolon.

reviewbotreviewbot

Col: 580 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 581 Missing semicolon.

reviewbotreviewbot

Col: 673 Missing semicolon.

reviewbotreviewbot

Col: 715 Missing semicolon.

reviewbotreviewbot

Col: 720 'i' was used before it was defined.

reviewbotreviewbot

Col: 560 Expected an identifier and instead saw '='.

reviewbotreviewbot

Col: 560 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 695 Missing semicolon.

reviewbotreviewbot

Col: 9 Do not use 'new' for side effects.

reviewbotreviewbot

Col: 7 'GifRecorder' is defined but never used.

reviewbotreviewbot

Col: 60 Missing semicolon.

reviewbotreviewbot

Col: 46 Missing semicolon.

reviewbotreviewbot

Col: 5 'GifEncoderAbstraction' was used before it was defined.

reviewbotreviewbot

Why is this change necessary, out of curiosity? It looks unrelated to your project.

mike_conleymike_conley

What kind of configurability are you envisioning here? The "For now" comment doesn't give us much to work with - ...

mike_conleymike_conley

Where is this file? I don't see it in the patch.

mike_conleymike_conley

What is the strange bug in Chrome?

mike_conleymike_conley

Please remove this newline.

mike_conleymike_conley

Please remove this newline.

mike_conleymike_conley

Can you please document each of these functions?

mike_conleymike_conley

Please remove this newline.

mike_conleymike_conley

This is how we generally do this sort of thing: https://github.com/reviewboard/reviewboard/blob/b637ecfcfff04fbe493fb6186e7be4af5745503e/reviewboard/static/rb/js/newReviewRequest/views/preCommitView.js#L10-L54 Note that it's important to keep the indentation for ...

mike_conleymike_conley

I noticed that you're putting the strings for other things inside GIF_COMMENT_UI_STRINGS. Why not these as well?

mike_conleymike_conley

You don't need to set a value to the disabled attribute.

mike_conleymike_conley

Can you invert this condition, so that we return if $mainContainer.length > 0, and then otherwise continue on below?

mike_conleymike_conley

Space after //

mike_conleymike_conley

Please remove this newline.

mike_conleymike_conley

Please remove this newline, and the one at the end of the function.

mike_conleymike_conley

Col: 9 Do not use 'new' for side effects.

reviewbotreviewbot

Please remove this newline.

mike_conleymike_conley

Space after //

mike_conleymike_conley

Col: 7 'GifRecorder' is defined but never used.

reviewbotreviewbot

Trailing whitespace

mike_conleymike_conley

Space after //

mike_conleymike_conley

Let's format this: navigator.mediaDevices.getUserMedia({ video: VIDEO_CONFIG, audio: false }).then(this.displayMediaStream.bind(this)) .catch(this.displayMediaStreamError.bind(this));

mike_conleymike_conley

Please remove this extra newline.

mike_conleymike_conley

Please remove this dead code.

mike_conleymike_conley

These extra newlines can go.

mike_conleymike_conley

Col: 5 'GifEncoderAbstraction' was used before it was defined.

reviewbotreviewbot

Extra newline can be removed.

mike_conleymike_conley

Extra newline can be removed.

mike_conleymike_conley

Maybe mention that they can be attached to Review Board comments, so: Record GIFs from your webcam and attach them ...

mike_conleymike_conley

Col: 7 'GifRecorder' is defined but never used.

reviewbotreviewbot

Col: 27 Expected '!==' and instead saw '!='.

reviewbotreviewbot

E501 line too long (89 > 79 characters)

reviewbotreviewbot

Please add a docstring.

daviddavid

Please add a docstring.

daviddavid

Please add a docstring.

daviddavid

Add another blank line here.

daviddavid

This only works with the default static media path. This should instead be: (at the top) from django.contrib.staticfiles.templatetags.staticfiles import static ...

daviddavid

Add a trailing comma here.

daviddavid

This should probably also be const instead of var

daviddavid

Doc comments like this should be in a very similar format to python docstrings. Only major difference is in the ...

daviddavid

Since this is ES6 we can use member function sugar: injectUiIfNotInDom() { } This should probably also have "UI" and ...

daviddavid

This can make use of the dedent JS template string processor: ``javascript const mainContainerHtml = dedent <div class="gif-container"> <div style="max-height: ...

daviddavid

Instead of doing function() {}.bind(this), you can use a fat arrow function, which have lexical scoping for this: $saveButton.click(ev => ...

daviddavid

This can be const.

daviddavid

This blank line can go away.

daviddavid

This blank line can go away.

daviddavid

Please fix up formatting of this docstring to match our doc standards.

daviddavid

Use ES6 member function sugar.

daviddavid

Please fix up formatting of this docstring to match our doc standards.

daviddavid

Use ES6 member function sugar.

daviddavid

Please fix docstring syntax.

daviddavid

Use ES6 member function sugar.

daviddavid

Instead of using inline style here, please just put it in the LESS file for the .gifcomment-control-open class. Also, please ...

daviddavid

This is a good place to use a fat arrow function.

daviddavid

Join these two lines together.

daviddavid

This is a good place to use a fat arrow function.

daviddavid

This should use jquery accessors: this.$toggleMainContainerButton .prop('disabled', !$enableMarkdownCheckbox.prop('checked'));

daviddavid

Since it's only used once, you can skip assigning this to a name. This is also a good place to ...

daviddavid

Add another blank line.

daviddavid

Can use ES6 function sugar.

daviddavid

We're moving away from _super because it has some issues. This should just be explicit: RB.Extension.prototype.initialize.call(this)

daviddavid

Add a trailing comma.

daviddavid

Add a trailing comma.

daviddavid

This isn't an ES6 file so it needs to use var. Or you could change it to be .es6.js.

daviddavid

Col: 7 'GifRecorder' is defined but never used.

reviewbotreviewbot

E501 line too long (97 > 79 characters)

reviewbotreviewbot

E501 line too long (92 > 79 characters)

reviewbotreviewbot

Col: 7 'GifRecorder' is defined but never used.

reviewbotreviewbot

This was probably copied from other code, but it uses an old format for docstrings. See https://www.notion.so/reviewboard/Writing-Codebase-Documentation-e16312b5f061437cb73cbfa369ac3cb5

daviddavid

Just confirming , if there's a micro or patch, we still bump up the micro version?

FL floriecai

Single quotes

daviddavid

Same here.

daviddavid

Same here.

daviddavid

The summary line of doc comments should be in the imperative mood (Create) rather than passive (Creates)

daviddavid

Wrap to 80 columns

daviddavid

Should be in the format of: Returns: jQuery: The newly created or previously existing DOM element... See https://www.notion.so/reviewboard/Writing-Codebase-Documentation-e16312b5f061437cb73cbfa369ac3cb5

daviddavid

Blank line after this.

daviddavid

Blank line after this.

daviddavid

This should be: ``javascript dedent <div class="gif-container"> </div> `;

daviddavid

Should be imperative mood ("Display")

daviddavid

Should be imperative mood ("Close")

daviddavid

Blank line after this.

daviddavid

Do we have string localization in our js code?

FL floriecai

Blank line after this.

daviddavid

Should be imperative mood ("Inject")

daviddavid

Wrap to 80 columns.

daviddavid

There's an extra space character at the end of this line.

daviddavid

ditto about localization

FL floriecai

Blank line after this.

daviddavid

Blank line after this.

daviddavid

Blank line after this.

daviddavid

Blank line after this.

daviddavid

Add a doc comment?

daviddavid

Should be imperative mood ("Create")

daviddavid

Should use "Args:" and "Returns:" format as described in our codebase docs guide.

daviddavid

Blank line after this.

daviddavid

Trailing comma.

daviddavid

Adds -> Add

daviddavid

Should use Args/Returns

daviddavid

Renders -> Render

daviddavid

Should use Args/Returns

daviddavid

Col: 7 'GifRecorder' is defined but never used.

reviewbotreviewbot

Checks -> Check Returns: boolean: Whether the current browser has the getUserMedia function.

daviddavid

Displays -> Display

daviddavid

Args:

daviddavid

Initializes -> Initialize

daviddavid

Shutdowns -> Shut down the

daviddavid

Triages and displays -> Triage and display

daviddavid

Wrap to 80 columns. We should also probably use dedent... here.

daviddavid

Should be } else {

daviddavid

Pops up -> Pop up

daviddavid

Let's wrap as: navigator.mediaDevices.getUserMedia({ ... }) .then(...) .catch(...);

daviddavid

Trailing comma.

daviddavid

Renders -> Render

daviddavid

Args:

daviddavid

Can be: this.$previewVideo.onloadedmetadata = () => { this.startRecButton.disabled = false; }

daviddavid

Enables -> Enable

daviddavid

Enables -> Enable These two lines also need to be indented 1 more space.

daviddavid

Disables -> Disable

daviddavid

Updates -> Update

daviddavid

Args:

daviddavid

Starts -> Start

daviddavid

Blank line after this.

daviddavid

Wrap to 80 columns.

daviddavid

Stops -> Stop

daviddavid

Blank line after this.

daviddavid

Use a fat arrow function instead of function(blob) {}.bind(this)

daviddavid

Args:

daviddavid

Wrap to 80 columns

daviddavid

These should be indented 4 spaces (currently they're indented 1).

daviddavid

This can be const-- or even better, just do return new GIF({ ... });

daviddavid

Col: 7 'GifRecorder' is defined but never used.

reviewbotreviewbot

Same here as my previous comment regarding indentation with dedent

daviddavid
Checks run (2 failed)
flake8 failed.
JSHint failed.

flake8

JSHint

rpolyano
Review request changed

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

rpolyano
Review request changed

Description:

   

This extension will allow users to capture short (3-5 second) videos of themselves, which are then encoded as animated GIFs client-side, and then uploaded and embedded into a comment on a code review.

   
~  

Currently, I have implemented a workflow in javascript which allows a user to record, stop, and re-record a video. The gif is held as a blob in memory for now.

  ~

The extension is now fully implemented for pop-over comments.

Testing Done:

~  

Proof of concept was manually tested in a standalone HTML page before it was ported and refactored into the reviewboard extension structure.

  ~

Test comments were posted on dummy reviews

  + Tried canceling out of recording in both preview and recording states by clicking cancel, unchecking and rechecking Enable Markdown

Commit:

-8605a2515cd6c2dc1b31395af0ccdbc051b86036
+92d189199adbc881c76fa69a90da92b222bfb8d9

Diff:

Revision 3 (+530)

Show changes

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

rpolyano
Review request changed

Testing Done:

   

Test comments were posted on dummy reviews

~   Tried canceling out of recording in both preview and recording states by clicking cancel, unchecking and rechecking Enable Markdown

  ~ Tried canceling out of recording in both preview and recording states by clicking cancel, unchecking and rechecking Enable Markdown
  + Tested extension functions in Chrome, Edge, and Firefox

Diff:

Revision 4 (+570 -1)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

rpolyano
rpolyano
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

mike_conley
  1. Hey Roman,

    Thanks! I think this looks pretty great. I have some cosmetic issues, but I think the overall architecture makes sense.

    See below for questions and suggestions.

    -Mike

  2. Why is this change necessary, out of curiosity? It looks unrelated to your project.

  3. What kind of configurability are you envisioning here? The "For now" comment doesn't give us much to work with - maybe should remove it for now.

    1. Was thinking of having size/length related parameters configurable at some point. I can remove it if that's not in the plans yet

    2. Let's remove it until we need it.

  4. Where is this file? I don't see it in the patch.

    1. I'm not sure how to properly put up third-party lib files. Suggestion on Slack was excluding them with rbt post -X <pattern> so they don't generate hundreds of JSHint errors.

    2. Ah, noted. I'll drop this.

  5. What is the strange bug in Chrome?

    1. Buttons with opacity 1.0 become invisible on top of a video element. I will make the comment more clear.

  6. Please remove this newline.

  7. Please remove this newline.

  8. Can you please document each of these functions?

  9. Please remove this newline.

  10. This is how we generally do this sort of thing:

    https://github.com/reviewboard/reviewboard/blob/b637ecfcfff04fbe493fb6186e7be4af5745503e/reviewboard/static/rb/js/newReviewRequest/views/preCommitView.js#L10-L54

    Note that it's important to keep the indentation for each of the nodes to make it easier to reason about what is parenting what.

  11. I noticed that you're putting the strings for other things inside GIF_COMMENT_UI_STRINGS. Why not these as well?

  12. You don't need to set a value to the disabled attribute.

  13. Can you invert this condition, so that we return if $mainContainer.length > 0, and then otherwise continue on below?

  14. Please remove this newline.

  15. Please remove this newline, and the one at the end of the function.

  16. Please remove this newline.

  17. Trailing whitespace

  18. rbgifcomments/rbgifcomments/static/js/gifrecord.js (Diff revision 5)
     
     
     
     
     
     
     

    Let's format this:

    navigator.mediaDevices.getUserMedia({
        video: VIDEO_CONFIG,
        audio: false
    }).then(this.displayMediaStream.bind(this))
      .catch(this.displayMediaStreamError.bind(this));
    
  19. Please remove this extra newline.

  20. rbgifcomments/rbgifcomments/static/js/gifrecord.js (Diff revision 5)
     
     
     
     
     
     
     
     
     

    Please remove this dead code.

  21. These extra newlines can go.

  22. Extra newline can be removed.

  23. Extra newline can be removed.

  24. rbgifcomments/setup.py (Diff revision 5)
     
     

    Maybe mention that they can be attached to Review Board comments, so:

    Record GIFs from your webcam and attach them to Review Board comments!

  25. 
      
rpolyano
Review request changed

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

rpolyano
Review request changed
david
  1. 
      
  2. Can you edit the description to explain where to get js/lib/gif.js ?

  3. rbgifcomments/rbgifcomments/__init__.py (Diff revision 7)
     
     

    Please add a docstring.

  4. rbgifcomments/rbgifcomments/__init__.py (Diff revision 7)
     
     

    Please add a docstring.

  5. rbgifcomments/rbgifcomments/__init__.py (Diff revision 7)
     
     

    Please add a docstring.

  6. Add another blank line here.

  7. This only works with the default static media path. This should instead be:

    (at the top)
    from django.contrib.staticfiles.templatetags.staticfiles import static
    
    
    (here)
    @property
    def js_worker_path(self):
        return static('ext/%s/js/lib/gif.worker.js' % self.id)
    
  8. Add a trailing comma here.

  9. This should probably also be const instead of var

  10. rbgifcomments/rbgifcomments/static/js/gifComments.es6.js (Diff revision 7)
     
     
     
     
     
     
     

    Doc comments like this should be in a very similar format to python docstrings. Only major difference is in the comment syntax:

    /**
     * Create the GIF recording UI.
     *
     * ... more explanation ...
     */
    

    (note the double ** on the first line and alignment of the *s).

    "Returns" should also be a "Returns:" section, and the type it returns is jQuery.

  11. Since this is ES6 we can use member function sugar:

    injectUiIfNotInDom() {
    }
    

    This should probably also have "UI" and "DOM" instead of "Ui" and "Dom".

  12. rbgifcomments/rbgifcomments/static/js/gifComments.es6.js (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    This can make use of the dedent JS template string processor:

    ``javascript const mainContainerHtml = dedent
    <div class="gif-container">
    <div style="max-height: 85%;">
    ...
    </div>
    </div>
    `;

    This way you can avoid all the string concatenation.

  13. Instead of doing function() {}.bind(this), you can use a fat arrow function, which have lexical scoping for this:

    $saveButton.click(ev => {
       ...
    });
    
  14. This blank line can go away.

  15. This blank line can go away.

  16. rbgifcomments/rbgifcomments/static/js/gifComments.es6.js (Diff revision 7)
     
     
     
     
     
     

    Please fix up formatting of this docstring to match our doc standards.

  17. Use ES6 member function sugar.

  18. rbgifcomments/rbgifcomments/static/js/gifComments.es6.js (Diff revision 7)
     
     
     
     
     
     

    Please fix up formatting of this docstring to match our doc standards.

  19. Use ES6 member function sugar.

  20. rbgifcomments/rbgifcomments/static/js/gifComments.es6.js (Diff revision 7)
     
     
     
     
     
     

    Please fix docstring syntax.

  21. Use ES6 member function sugar.

  22. Instead of using inline style here, please just put it in the LESS file for the .gifcomment-control-open class.

    Also, please try to wrap to 80 columns.

  23. This is a good place to use a fat arrow function.

  24. Join these two lines together.

  25. This is a good place to use a fat arrow function.

  26. This should use jquery accessors:

    this.$toggleMainContainerButton
        .prop('disabled', !$enableMarkdownCheckbox.prop('checked'));
    
  27. rbgifcomments/rbgifcomments/static/js/gifComments.es6.js (Diff revision 7)
     
     
     
     
     
     

    Since it's only used once, you can skip assigning this to a name. This is also a good place to use a fat arrow function.

  28. Add another blank line.

  29. Can use ES6 function sugar.

  30. We're moving away from _super because it has some issues. This should just be explicit: RB.Extension.prototype.initialize.call(this)

  31. Add a trailing comma.

  32. Add a trailing comma.

  33. rbgifcomments/rbgifcomments/static/js/gifrecord.js (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    This isn't an ES6 file so it needs to use var. Or you could change it to be .es6.js.

  34. 
      
rpolyano
Review request changed

Change Summary:

Implemented code style and documentation feedback

Description:

   

This extension will allow users to capture short (3-5 second) videos of themselves, which are then encoded as animated GIFs client-side, and then uploaded and embedded into a comment on a code review.

   
   

The extension is now fully implemented for pop-over comments.

  +
  +

The gif.js library and worker (which are excluded from review) can be found here: https://github.com/jnordberg/gif.js/tree/master/dist

  + They should be placed in \static\js\lib\gif.js and \static\js\lib\gif.worker.js

Diff:

Revision 8 (+734)

Show changes

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

rpolyano
Review request changed
david
  1. 
      
  2. rbgifcomments/rbgifcomments/__init__.py (Diff revision 9)
     
     
     
     
     
     
     

    This was probably copied from other code, but it uses an old format for docstrings. See https://www.notion.so/reviewboard/Writing-Codebase-Documentation-e16312b5f061437cb73cbfa369ac3cb5

  3. rbgifcomments/rbgifcomments/__init__.py (Diff revision 9)
     
     

    Single quotes

  4. rbgifcomments/rbgifcomments/__init__.py (Diff revision 9)
     
     
     
     
     
     

    Same here.

  5. rbgifcomments/rbgifcomments/__init__.py (Diff revision 9)
     
     
     
     

    Same here.

  6. The summary line of doc comments should be in the imperative mood (Create) rather than passive (Creates)

  7. rbgifcomments/rbgifcomments/static/js/gifComments.es6.js (Diff revision 9)
     
     
     
     
     
     

    Wrap to 80 columns

  8. Should be in the format of:

    Returns:
        jQuery:
        The newly created or previously existing DOM element...
    

    See https://www.notion.so/reviewboard/Writing-Codebase-Documentation-e16312b5f061437cb73cbfa369ac3cb5

  9. Blank line after this.

  10. Blank line after this.

  11. rbgifcomments/rbgifcomments/static/js/gifComments.es6.js (Diff revision 9)
     
     
     
     
     
     
     

    This should be:

    ``javascript dedent
    <div class="gif-container">
    </div>
    `;

    1. The markdown is messing up your comment I am not sure how you're trying to format it differently from what I have.

    2. OK, without code block:

      dedent`
      <div class="gif-container">
      ...
      </div>
      `;

  12. Should be imperative mood ("Display")

  13. Should be imperative mood ("Close")

  14. Blank line after this.

  15. Blank line after this.

  16. Should be imperative mood ("Inject")

  17. Wrap to 80 columns.

  18. There's an extra space character at the end of this line.

  19. Blank line after this.

  20. Blank line after this.

  21. Blank line after this.

  22. Blank line after this.

  23. Should be imperative mood ("Create")

  24. Should use "Args:" and "Returns:" format as described in our codebase docs guide.

  25. Blank line after this.

  26. Should use Args/Returns

  27. Should use Args/Returns

  28. Checks -> Check

    Returns:
    boolean:
    Whether the current browser has the getUserMedia function.

  29. Displays -> Display

  30. Initializes -> Initialize

  31. Shutdowns -> Shut down the

  32. Triages and displays -> Triage and display

  33. rbgifcomments/rbgifcomments/static/js/gifrecord.es6.js (Diff revision 9)
     
     
     
     
     
     

    Wrap to 80 columns. We should also probably use dedent... here.

  34. Should be } else {

  35. rbgifcomments/rbgifcomments/static/js/gifrecord.es6.js (Diff revision 9)
     
     
     
     
     
     

    Let's wrap as:

    navigator.mediaDevices.getUserMedia({
        ...
    })
        .then(...)
        .catch(...);
    
  36. Can be:

    this.$previewVideo.onloadedmetadata = () => {
        this.startRecButton.disabled = false;
    }
    
  37. Enables -> Enable

    These two lines also need to be indented 1 more space.

  38. Disables -> Disable

  39. Blank line after this.

  40. Wrap to 80 columns.

  41. Blank line after this.

  42. Use a fat arrow function instead of function(blob) {}.bind(this)

  43. rbgifcomments/rbgifcomments/static/js/gifrecord.es6.js (Diff revision 9)
     
     
     
     
     
     
     
  44. Wrap to 80 columns

  45. 
      
FL
  1. 
      
  2. rbgifcomments/rbgifcomments/__init__.py (Diff revision 9)
     
     

    Just confirming , if there's a micro or patch, we still bump up the micro version?

    1. I assume so, but would be good to get some official versioning guidelines

  3. Do we have string localization in our js code?

    1. Yes, using gettext, but it will be a separate work item. From what I can tell, the localization will happen server-side when "rendering" the JS code.

  4. ditto about localization

  5. 
      
rpolyano
Review request changed
david
  1. 
      
  2. These should be indented 4 spaces (currently they're indented 1).

  3. This can be const-- or even better, just do return new GIF({ ... });

  4. rbgifcomments/rbgifcomments/static/js/gifrecord.es6.js (Diff revision 10)
     
     
     
     
     
     
     
     

    Same here as my previous comment regarding indentation with dedent

  5. 
      
rpolyano
Review request changed
Loading...