[Emoji UI Picker] [PART 1 | Review Board] Finished working implementation

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

Information

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

Reviewers

[Emoji UI Picker] [PART 1 | Review Board] Finished working implementation

=> Still need to add unit tests for frontend

See Final Report

Image

=> Local testing
=> Unit tests needed for front-end

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
Commit:
af0ae5bd81367fed4c2a991badfb08b034eabcd1
c2950ae01ab3f1ee445cea0b18ba2ec9767ec392

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)
     
     
     
     
    Show all issues

    Keep these alphabetized please.

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

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

  4. Show all issues

    Combine these together (} else {)

  5. Show all issues

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

  6. Show all issues

    Parens aren't necessary around e

  7. Show all issues

    Can use Object.entries() here too

  8. Show all issues

    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. Show all issues

    We can do:

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

David's comments

Commit:
c2950ae01ab3f1ee445cea0b18ba2ec9767ec392
5725227af57d4baea2fffd32c07ec54994dda396

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

matthewgoodman13
Review request changed
Commit:
5725227af57d4baea2fffd32c07ec54994dda396
c9ffcba3d3ee260b1d63e8c257c2642adeccadbb

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

matthewgoodman13
Review request changed
Commit:
c9ffcba3d3ee260b1d63e8c257c2642adeccadbb
0c6e38ef87bc6718e50374753b2cd95bfe5b1d05

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

matthewgoodman13
matthewgoodman13
Review request changed
Summary:
[WIP | Emoji UI Picker] [PART 1 | Review Board] Working implementation ready for review
[Emoji UI Picker] [PART 1 | Review Board] Finished working implementation
Description:
~  

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

  ~

[Emoji UI Picker] [PART 1 | Review Board] Finished working implementation

   
~  

=> Would like a preliminary review

  ~

=> Still need to add unit tests for frontend

-   => Still need to add unit tests

   
~  

IMAGES:

~   Image

  ~

See Final Report

  ~
  +

Image

Testing Done:
   

=> Local testing

~   => Unit tests needed

  ~ => Unit tests needed for front-end