The EmojiPicker stores state in global module-level variables, which causes issues when multiple emoji pickers are present on the same page. schungx suggests moving these variables into the class definition to make them instance-specific fields. joffreyjaffeux agrees to refactor the code, citing his inexperience with JavaScript and Discourse at the time of writing.
schungx explains that the issue arises from the assumption that only one emoji picker would be present on a page, which is broken by plugins like discourse/babble and discourse/retort. joffreyjaffeux acknowledges this and schungx provides guidance on how to fix the issue.
Additionally, schungx points out a specific commit that introduced the problem and schungx suggests optimizing the emoji picker by using a single PNG image with sprites, which would improve user experience. However, codinghorror disagrees, citing the use of HTTP/2, which makes sprite sheets unnecessary. schungx counters that not everyone uses HTTP/2, but codinghorror argues that Discourse’s default Docker image runs HTTP/2, making it a non-issue.
The following variables are used for emoji-pickers, but they are currently module-GLOBAL variables instead of instance fields.
let $picker, $filter, $results, $list, scrollPosition, $visibleSections, _checkTimeout;
In particker, $picker is used to popup the emoji-picker. It is a module-global variable, so what that means is that there CANNOT BE any permanent emoji-picker’s on the page (e.g. discourse/babble and discourse/retort both put emoji-pickers permanently on the page).
When the compose-reply window is open, the standard emoji picker is created, overwriting $picker for everybody, causing all permanent emoji-pickers on the page to break.
Suggestion is to move all these variables into the class definition, and make them instance-specific fields. This should then allow for multiple emoji-pickers to co-exist on the same page with conflicts.
When the code was written, it was probably assumed that only one emoji picker will ever be present on a page. A reasonable assumption actually, except that we’ll trust plugin authors to promptly break all our assumptions!
You are putting out the emoji’s as a huge bunch of PNG image files. When the emoji-picker pop’s up, the browser has to start fetching a huge number of very small PNG files. This affects user experience.
A much better idea is to have one single PNG image with all these sprites. Then use a class to fetch the relevant background-position from CSS. This should make the emoji’s show up much quicker.
So:
Standard emoji’s: put emoji name in class, use CSS to refer to background image and background-position to display it
Custom emoji’s: continue to put emoji name in background-url
Incorrect, sprite sheets are a HTTP/1.1 optimization. This is also a fragile and complex techinque that breaks, say, simply linking to a given emoji. HTTP 2, which we already use and have for quite a few months, doesn’t require these contortions.
Now the advice “only load what is (currently) visible” is timeless, and damn I wish web browsers had implemented this back in 2002. (Try loading a long web page with 50 images, when 49 of them are not visible due to scroll position.)
@codinghorror You’re right! Using Let's Encrypt makes it really a no-brainer…
Since it is such a no-brainer to setup HTTP for Discourse, I agree that using HTTP/2 best practices is better, except in the case where somebody is behind a corporate proxy that is very very outdated and doesn’t support HTTP/2.
In other words, there really should not be many Discourse sites out there not running on HTTPS, right?
