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;
$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/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.
Yes it could probably use some refactoring, will look at it.
@joffreyjaffeux Thanks for looking into this.
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!
this and also I was quite new to js and discourse
No worries mate. JS can be a b**ch to pick up.
What you really need is to move those module-global variables into the class definition, so they are class-instance properties.
Then change all the access from
Then Bob’s your uncle!
P.S. I could have made the changes myself and submit a PR, but I got scared of forking the entire Discourse repo just to test such small changes…
No worries I will improve this.
Your commit here is what’s breaking everything:
You attempted to optimize by caching some values, but caching them into module-global variables. Should have cached them into instance properties.
Oh, by the way, a not very related suggestion:
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.
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
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.)
I agree, but old practices are there for a reason.
Not everybody runs HTTP/2. For example, those behind a corporate proxy may be stuck for 1.1 for a long time.
And it seems like those sites not running HTTPS are not served HTTP/2. There are still lots of sites not on HTTPS.
The default Discourse docker image runs http/2, see for yourself at
discourse.codinghorror.com etc. https://tools.keycdn.com/http2-test
This is one of the big, big advantages of Docker packaging – we can give people an optimal setup right out of the box.
@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?