Summary

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

Review Request #11879 — Created Nov. 21, 2021 and updated Dec. 6, 2021, 10:09 p.m.

Information

Owner

matthewgoodman13

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

Commit

0c6e38e...

Reviewers

Groups

reviewboard

People

Description

[Emoji UI Picker] [PART 1 | Review Board] Finished working implementation
=> Still need to add unit tests for frontend
See Final Report

Testing Done

=> Local testing

=> Unit tests needed for front-end

Issues

Description	From	Last Updated
E266 too many leading '#' for block comment	reviewbot	Nov. 21, 2021, 9:32 p.m.
E501 line too long (93 > 79 characters)	reviewbot	Nov. 21, 2021, 9:32 p.m.
E302 expected 2 blank lines, found 0	reviewbot	Nov. 21, 2021, 9:32 p.m.
W291 trailing whitespace	reviewbot	Nov. 21, 2021, 9:32 p.m.
W291 trailing whitespace	reviewbot	Nov. 21, 2021, 9:32 p.m.
Col: 33 Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. (categories, category)	reviewbot	Nov. 21, 2021, 9:32 p.m.
Col: 11 Missing semicolon.	reviewbot	Nov. 21, 2021, 9:32 p.m.
Col: 40 Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. (key)	reviewbot	Nov. 21, 2021, 9:32 p.m.
Col: 19 Missing semicolon.	reviewbot	Nov. 21, 2021, 9:32 p.m.
Col: 33 Expected '{' and instead saw 'this'.	reviewbot	Nov. 21, 2021, 9:32 p.m.
Col: 15 Missing semicolon.	reviewbot	Nov. 21, 2021, 9:32 p.m.
E302 expected 2 blank lines, found 1	reviewbot	Nov. 21, 2021, 9:32 p.m.
E305 expected 2 blank lines after class or function definition, found 1	reviewbot	Nov. 21, 2021, 9:32 p.m.
W292 no newline at end of file	reviewbot	Nov. 21, 2021, 9:32 p.m.
E266 too many leading '#' for block comment	reviewbot	Nov. 24, 2021, 9:44 p.m.
E501 line too long (93 > 79 characters)	reviewbot	Nov. 24, 2021, 9:44 p.m.
Keep these alphabetized please.	david	Nov. 24, 2021, 8:19 p.m.
You've got a mix of naming conventions here. JS code should always use camelCase	david	Nov. 24, 2021, 8:21 p.m.
Combine these together (} else {)	david	Nov. 24, 2021, 8:24 p.m.
Our convention is that any jQuery object names should have $ in them. Let's call this $newImg	david	Nov. 24, 2021, 8:24 p.m.
Col: 33 Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. (categories, category)	reviewbot	Nov. 24, 2021, 9:44 p.m.
Parens aren't necessary around e	david	Nov. 24, 2021, 8:27 p.m.
Can use Object.entries() here too	david	Nov. 24, 2021, 8:34 p.m.
Col: 40 Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. (key)	reviewbot	Nov. 24, 2021, 9:44 p.m.
Adding "Error" doesn't help here--the console will already show it as an error. If we're going to add clarification text …	david	Nov. 24, 2021, 8:34 p.m.
We can do: for (let [alias, shortcode] in Object.entries(aliases)) { }	david	Nov. 24, 2021, 8:38 p.m.
E266 too many leading '#' for block comment	reviewbot	Nov. 28, 2021, 12:38 p.m.
E501 line too long (93 > 79 characters)	reviewbot	Nov. 28, 2021, 12:38 p.m.
Col: 33 Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. (categories, category)	reviewbot	Nov. 28, 2021, 12:38 p.m.
W293 blank line contains whitespace	reviewbot	Nov. 28, 2021, 12:38 p.m.
E501 line too long (82 > 79 characters)	reviewbot	Nov. 28, 2021, 12:38 p.m.
E501 line too long (113 > 79 characters)	reviewbot	Nov. 28, 2021, 12:38 p.m.
E266 too many leading '#' for block comment	reviewbot	Nov. 28, 2021, 12:40 p.m.
E501 line too long (93 > 79 characters)	reviewbot	Nov. 28, 2021, 12:40 p.m.
Col: 33 Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. (categories, category)	reviewbot	Nov. 28, 2021, 12:40 p.m.
W293 blank line contains whitespace	reviewbot	Nov. 28, 2021, 12:40 p.m.
E501 line too long (84 > 79 characters)	reviewbot	Nov. 28, 2021, 12:40 p.m.
E501 line too long (82 > 79 characters)	reviewbot	Nov. 28, 2021, 12:40 p.m.
E501 line too long (93 > 79 characters)	reviewbot	Nov. 28, 2021, 12:40 p.m.
E266 too many leading '#' for block comment	reviewbot	Nov. 28, 2021, 1:06 p.m.
E501 line too long (93 > 79 characters)	reviewbot	Nov. 28, 2021, 1:06 p.m.
Col: 33 Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. (categories, category)	reviewbot	Nov. 28, 2021, 1:07 p.m.
E501 line too long (82 > 79 characters)	reviewbot	Nov. 28, 2021, 1:07 p.m.
E501 line too long (93 > 79 characters)	reviewbot	Nov. 28, 2021, 1:07 p.m.

flake8 failed.

JSHint failed.

flake8

reviewboard/reviews/markdown_utils.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E266 too many leading '#' for block comment
```
reviewboard/reviews/markdown_utils.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E501 line too long (93 > 79 characters)
```
reviewboard/reviews/markdown_utils.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E302 expected 2 blank lines, found 0
```
reviewboard/reviews/markdown_utils.py (Diff revision 1)
The issue has been dropped. Show all issues
```
W291 trailing whitespace
```
reviewboard/reviews/markdown_utils.py (Diff revision 1)
The issue has been dropped. Show all issues
```
W291 trailing whitespace
```
reviewboard/webapi/resources/emoji.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E302 expected 2 blank lines, found 1
```
reviewboard/webapi/resources/emoji.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E305 expected 2 blank lines after class or function definition, found 1
```
reviewboard/webapi/resources/emoji.py (Diff revision 1)
The issue has been dropped. Show all issues
```
W292 no newline at end of file
```

JSHint

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

The issue has been dropped. Show all issues

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

reviewboard/static/rb/js/ui/views/emojiPickerView.es6.js (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 11
 Missing semicolon.
```

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

The issue has been dropped. Show all issues

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

reviewboard/static/rb/js/ui/views/emojiPickerView.es6.js (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 19
 Missing semicolon.
```
reviewboard/static/rb/js/ui/views/emojiPickerView.es6.js (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 33
 Expected '{' and instead saw 'this'.
```
reviewboard/static/rb/js/ui/views/emojiPickerView.es6.js (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 15
 Missing semicolon.
```

Commit:

af0ae5bd81367fed4c2a991badfb08b034eabcd1

c2950ae01ab3f1ee445cea0b18ba2ec9767ec392

Diff:

Revision 2 (+700 -11)

Show changes

	reviewboard/staticbundles.py
	reviewboard/reviews/markdown_utils.py
	reviewboard/static/rb/css/ui/emoji-picker.less
	reviewboard/static/rb/js/ui/views/emojiPickerView.es6.js
	reviewboard/static/rb/js/ui/views/textEditorView.es6.js
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/emoji.py
	reviewboard/webapi/resources/root.py

Checks run (2 failed)

flake8 failed.

JSHint failed.

flake8

reviewboard/reviews/markdown_utils.py (Diff revision 2)
The issue has been dropped. Show all issues
```
E266 too many leading '#' for block comment
```
reviewboard/reviews/markdown_utils.py (Diff revision 2)
The issue has been dropped. Show all issues
```
E501 line too long (93 > 79 characters)
```

JSHint

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

The issue has been dropped. Show all issues

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

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

The issue has been dropped. Show all issues

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

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

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

IMAGES:

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

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

matthewgoodman13

Nov. 22, 2021, 8:33 a.m.

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.

reviewboard/reviews/markdown_utils.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Keep these alphabetized please.
```

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

The issue has been resolved. Show all issues

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

reviewboard/static/rb/js/ui/views/emojiPickerView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Combine these together (} else {)
```
reviewboard/static/rb/js/ui/views/emojiPickerView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Our convention is that any jQuery object names should have $ in them. Let's call this $newImg
```
reviewboard/static/rb/js/ui/views/emojiPickerView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Parens aren't necessary around e
```
reviewboard/static/rb/js/ui/views/emojiPickerView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Can use Object.entries() here too
```

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

The issue has been resolved. 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.

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

The issue has been resolved. Show all issues

We can do:

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

Change Summary:

David's comments

Commit:

c2950ae01ab3f1ee445cea0b18ba2ec9767ec392

5725227af57d4baea2fffd32c07ec54994dda396

Diff:

Revision 3 (+729 -13)

Show changes

	reviewboard/staticbundles.py
	reviewboard/reviews/markdown_utils.py
	reviewboard/static/rb/css/ui/emoji-picker.less
	reviewboard/static/rb/js/ui/views/emojiPickerView.es6.js
	reviewboard/static/rb/js/ui/views/textEditorView.es6.js
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/emoji.py
	reviewboard/webapi/resources/root.py

Checks run (2 failed)

flake8 failed.

JSHint failed.

flake8

reviewboard/reviews/markdown_utils.py (Diff revision 3)
The issue has been dropped. Show all issues
```
E266 too many leading '#' for block comment
```
reviewboard/reviews/markdown_utils.py (Diff revision 3)
The issue has been dropped. Show all issues
```
E501 line too long (93 > 79 characters)
```
reviewboard/webapi/resources/emoji.py (Diff revision 3)
The issue has been dropped. Show all issues
```
W293 blank line contains whitespace
```
reviewboard/webapi/resources/emoji.py (Diff revision 3)
The issue has been dropped. Show all issues
```
E501 line too long (82 > 79 characters)
```
reviewboard/webapi/resources/emoji.py (Diff revision 3)
The issue has been dropped. Show all issues
```
E501 line too long (113 > 79 characters)
```

JSHint

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

The issue has been dropped. Show all issues

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

Commit:

5725227af57d4baea2fffd32c07ec54994dda396

c9ffcba3d3ee260b1d63e8c257c2642adeccadbb

Diff:

Revision 4 (+733 -13)

Show changes

	reviewboard/staticbundles.py
	reviewboard/reviews/markdown_utils.py
	reviewboard/static/rb/css/ui/emoji-picker.less
	reviewboard/static/rb/js/ui/views/emojiPickerView.es6.js
	reviewboard/static/rb/js/ui/views/textEditorView.es6.js
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/emoji.py
	reviewboard/webapi/resources/root.py

Checks run (2 failed)

flake8 failed.

JSHint failed.

flake8

reviewboard/reviews/markdown_utils.py (Diff revision 4)
The issue has been dropped. Show all issues
```
E266 too many leading '#' for block comment
```
reviewboard/reviews/markdown_utils.py (Diff revision 4)
The issue has been dropped. Show all issues
```
E501 line too long (93 > 79 characters)
```
reviewboard/webapi/resources/emoji.py (Diff revision 4)
The issue has been dropped. Show all issues
```
W293 blank line contains whitespace
```
reviewboard/webapi/resources/emoji.py (Diff revision 4)
The issue has been dropped. Show all issues
```
E501 line too long (84 > 79 characters)
```
reviewboard/webapi/resources/emoji.py (Diff revision 4)
The issue has been dropped. Show all issues
```
E501 line too long (82 > 79 characters)
```
reviewboard/webapi/resources/emoji.py (Diff revision 4)
The issue has been dropped. Show all issues
```
E501 line too long (93 > 79 characters)
```

JSHint

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

The issue has been dropped. Show all issues

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

Commit:

c9ffcba3d3ee260b1d63e8c257c2642adeccadbb

0c6e38ef87bc6718e50374753b2cd95bfe5b1d05

Diff:

Revision 5 (+733 -13)

Show changes

	reviewboard/staticbundles.py
	reviewboard/reviews/markdown_utils.py
	reviewboard/static/rb/css/ui/emoji-picker.less
	reviewboard/static/rb/js/ui/views/emojiPickerView.es6.js
	reviewboard/static/rb/js/ui/views/textEditorView.es6.js
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/emoji.py
	reviewboard/webapi/resources/root.py

Checks run (2 failed)

flake8 failed.

JSHint failed.

flake8

reviewboard/reviews/markdown_utils.py (Diff revision 5)
The issue has been dropped. Show all issues
```
E266 too many leading '#' for block comment
```
reviewboard/reviews/markdown_utils.py (Diff revision 5)
The issue has been dropped. Show all issues
```
E501 line too long (93 > 79 characters)
```
reviewboard/webapi/resources/emoji.py (Diff revision 5)
The issue has been dropped. Show all issues
```
E501 line too long (82 > 79 characters)
```
reviewboard/webapi/resources/emoji.py (Diff revision 5)
The issue has been dropped. Show all issues
```
E501 line too long (93 > 79 characters)
```

JSHint

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

The issue has been dropped. Show all issues

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

Description:: [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:
~
~
-

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:
~
~
See Final Report
~
+
Testing Done:: => Local testing
~ => Unit tests needed
~ => Unit tests needed for front-end

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

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

		IMAGES:

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