search

clarkwinkelmann / flarum-ext-emojionearea

Add EmojioneArea emoji picker to Flarum
https://discuss.flarum.org/d/4787
MIT License
27 stars 12 forks source link

Davis' Code Review #5

Closed dav-is closed 7 years ago

dav-is commented 7 years ago

Good work! Thought you'd appriciate some feedback. Here's what I found:

Should probably extract to separate class: https://github.com/clarkwinkelmann/flarum-ext-emojionearea/blob/master/bootstrap.php#L12

Maybe this should be a div? https://github.com/clarkwinkelmann/flarum-ext-emojionearea/blob/master/js/forum/src/components/EmojiAreaButton.js#L19

Use const: https://github.com/clarkwinkelmann/flarum-ext-emojionearea/blob/master/js/forum/src/components/EmojiAreaButton.js#L26

Use const and send texteditor as parameter: https://github.com/clarkwinkelmann/flarum-ext-emojionearea/blob/master/js/forum/src/main.js#L13

clarkwinkelmann commented 7 years ago

Thanks ! I think I'll do all that :+1:

I still have to read some doc about that const thing because I've never actually used it :grimacing:

dav-is commented 7 years ago

Let me know if you need help achieving any of these or just in general :)

clarkwinkelmann commented 7 years ago

@dav-is I could do with some advice. I'm waiting for Gitter to come back online :see_no_evil:

dav-is commented 7 years ago

Which one are you having issues with?

clarkwinkelmann commented 7 years ago

Thanks again for your help, it's been very helpful :wink:

dav-is commented 7 years ago

:+1: Anytime! :)