[WIP | Emoji UI Picker] [PART 1 | Review Board] Working implementation ready for review

Review Request #11879 — Created Nov. 21, 2021 and updated

matthewgoodman13
Review Board
release-4.0.x
0c6e38e...
reviewboard

[WIP | Emoji UI Picker] [PART 1 | Review Board] Working implementation ready for review

=> Would like a preliminary review
=> Still need to add unit tests

IMAGES:
Image
Image

=> Local testing
=> Unit tests needed

Description From Last Updated

E266 too many leading '#' for block comment

reviewbotreviewbot

E501 line too long (93 > 79 characters)

reviewbotreviewbot

E302 expected 2 blank lines, found 0

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

Col: 33 Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. (categories, category)

reviewbotreviewbot

Col: 11 Missing semicolon.

reviewbotreviewbot

Col: 40 Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. (key)

reviewbotreviewbot

Col: 19 Missing semicolon.

reviewbotreviewbot

Col: 33 Expected '{' and instead saw 'this'.

reviewbotreviewbot

Col: 15 Missing semicolon.

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

E305 expected 2 blank lines after class or function definition, found 1

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

E266 too many leading '#' for block comment

reviewbotreviewbot

E501 line too long (93 > 79 characters)

reviewbotreviewbot

Keep these alphabetized please.

daviddavid

You've got a mix of naming conventions here. JS code should always use camelCase

daviddavid

Combine these together (} else {)

daviddavid

Our convention is that any jQuery object names should have $ in them. Let's call this $newImg

daviddavid

Col: 33 Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. (categories, category)

reviewbotreviewbot

Parens aren't necessary around e

daviddavid

Can use Object.entries() here too

daviddavid

Col: 40 Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. (key)

reviewbotreviewbot

Adding "Error" doesn't help here--the console will already show it as an error. If we're going to add clarification text ...

daviddavid

We can do: for (let [alias, shortcode] in Object.entries(aliases)) { }

daviddavid

E266 too many leading '#' for block comment

reviewbotreviewbot

E501 line too long (93 > 79 characters)

reviewbotreviewbot

Col: 33 Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. (categories, category)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E501 line too long (113 > 79 characters)

reviewbotreviewbot

E266 too many leading '#' for block comment

reviewbotreviewbot

E501 line too long (93 > 79 characters)

reviewbotreviewbot

Col: 33 Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. (categories, category)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E501 line too long (93 > 79 characters)

reviewbotreviewbot

E266 too many leading '#' for block comment

reviewbotreviewbot

E501 line too long (93 > 79 characters)

reviewbotreviewbot

Col: 33 Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. (categories, category)

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E501 line too long (93 > 79 characters)

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

flake8

JSHint

matthewgoodman13
Review request changed

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

matthewgoodman13
mxwang
  1. 
      
  2. This looks really sick!
    I'm curious, is there a reason you wrote this in JS instead of Python?

    1. Was replicating one of the infobox popups which were done similarly. Since, my picker was going to be used quite differently, I decided just to make my own View and then just simply attach it to the text editor. Having this client side is also beneficial so I don't have to load the emoji index every single time as it can be cached in the user's localStorage.

  3. 
      
david
  1. 
      
  2. reviewboard/reviews/markdown_utils.py (Diff revision 2)
     
     
     
     

    Keep these alphabetized please.

  3. reviewboard/static/rb/js/ui/views/emojiPickerView.es6.js (Diff revision 2)
     
     
     
     
     
     
     

    You've got a mix of naming conventions here. JS code should always use camelCase

  4. Combine these together (} else {)

  5. Our convention is that any jQuery object names should have $ in them. Let's call this $newImg

  6. Parens aren't necessary around e

  7. Can use Object.entries() here too

  8. Adding "Error" doesn't help here--the console will already show it as an error. If we're going to add clarification text we should probably indicate something about what we were trying to do when the error occurred.

  9. We can do:

    for (let [alias, shortcode] in Object.entries(aliases)) {
    }
    
  10. 
      
matthewgoodman13
Review request changed

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

matthewgoodman13
Review request changed

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

matthewgoodman13
Review request changed

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

Loading...