- 🇩🇪Germany IT-Cru Munich
Patch #46 worked so far for me, but I see some possible improvements we could maybe also integrate.
Split disqus.js and disqus.settings.js in to different libraries definitions, because disqus.settings.js seems to be only required for Drupal's Disqus settings form.
Switch from var to let/const syntax in all JS files.
Try to remove jQuery once and if possible also jQuery dependency from disqus.js to avoid extra loading of jQuery.
Simplify allif (settings.disqus.language || false) {
toif (settings.disqus.language) {
.
For better readability add extra lines between each libraries definition in disqus.libraries.yml file.
It seemsvar disqus_lazy_load = 0;
is never used.@ckng: what do you think about possible improvements?
I've also linked related issue where already in the past some work was done to remove jQuery from disqus.js.
- 🇨🇦Canada jigarius Montréal
@ IT-Cru IMHO if we have some improvement which is ready to be merged, we should include that as it is for now. I say so because this issue has been open for a long time now. Every time there is a working patch, some other thing changes in the module which makes the patch become stale. So, if we have it working this time, we should merge what we have so that we at least remove of the jQuery dependency. Later, we can create another PR to separate the admin JS from the user-facing JS.
What do you think?
- 🇩🇪Germany IT-Cru Munich
@jigarius: Complete fine for me. This is why I'm not switched it back to needs work and only mentioned possible additional improvements I see for near future fixing issues. It also make it easier when it is merged and rebase existing MRs like added linked issue which already remove usage of jQuery.
Better to split other improvements into small tasks which could be easier tested :)