Is there any possibly way to get Disqus to be lazy loaded in Drupal? Similar to this: http://www.myatus.com/2011/03/20/lazy-loading-disqus-in-wordpress/
I noticed that sites such as engadget are now lazyloading Disqus. Would be a great feature to have.
Proposed solution
* Allow admin to choose if Disqus should be lazy-loaded
* There can be 2 types of lazy-loading: on scroll and on click
* Lazy loading on scroll implies that Disqus is loaded when the user scrolls the disqus thread into view
** Preferably use IntersectionObserver API for better performance
** All JS from Disqus is not very performance-optimized, so at least we do our part
* Lazy loading on click implies that there will be a button showing the number of comments available – when clicked, the Disqus widget will be displayed
Comment | File | Size | Author |
---|---|---|---|
#47 | disqus-1508786-lazyload-intersectionobserver-47-1.x.patch | 10.81 KB | ckng |
#46 | disqus-1508786-lazyload-intersectionobserver-46-2.0.x.patch | 9.14 KB | ckng |
#42 | disqus-lazy-loading-1508786-42.patch | 11.46 KB | asrob |
#38 | disqus-lazy-loading-1508786-38.patch | 11.84 KB | asrob |
#37 | disqus-lazy-loading-1508786-37.patch | 13.62 KB | asrob |
Comments
Comment #1
RobLoachWould definitely speed up script performance to have it load asynchronously. I believe 7.x does some stuff to speed it up, but would be good to make sure it's in both 6 and 7.
Comment #2
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedI just ran into this. It is a necessary feature in my opinion.
Comment #3
joecanti CreditAttribution: joecanti commentedThis would be great - its quite a large script.
But maybe it would be better aimed at a higher level - like lazyload blocks, or lazyload panel panes? That way a generic solution could be used for other stuff??
Thanks, Joe
Comment #4
gregnostic CreditAttribution: gregnostic commentedAttached is a patch to enable lazy loading of the Disqus comments script. I've added configuration options to enable/disable lazy loading, choose whether you want to lazy load the script based on when the comments scroll into view or whether the user clicks on an HTML element, and the class for elements to attach the click handler to.
We've had an internal review of this code, but another pair of eyes looking over this would be appreciated!
Comment #5
gregnostic CreditAttribution: gregnostic commentedRe-rolled the patch with changes based on internal feedback.
Comment #6
slashrsm CreditAttribution: slashrsm commentedThanks! Looks OK but needs to go into D8 branch first. Here is quick port. Tested it really briefly and it seems to work. Some more testing would be more than welcome.
Comment #7
JayeshSolanki CreditAttribution: JayeshSolanki commentedTested the patch for both the branches; lazy loading works great!
but I see that the disqus comment count did not load. Please check if this bug could be reproduced.
just some minor nitpicks
missing semicolon
needs an extra space after $data['lazyLoadMethod'] = 'scroll'; (in d8 patch)
Comment #8
JayeshSolanki CreditAttribution: JayeshSolanki commentedComment #9
slashrsm CreditAttribution: slashrsm commentedFixed nitpiks.
Count bug is unrelated to this patch. It doesn't work even in HEAD. I suspect this problem is related to the way D8 loads links (via AJAX request). Currently we load count only once. We probably need to load it after AJAX request.
Comment #10
slashrsm CreditAttribution: slashrsm commented#2334865: Comment count script not working
Comment #11
vincer CreditAttribution: vincer commentedIt seems that this is, in part, for disqus-7.x-1.x-dev. Is there an order in which these patches should be applied? I'm just not clear on how to apply these files.
Comment #12
dawehnerNeeds config schema
I'm wondering whether we should validate shortname to ensure that we don't request an arbitrary external script?
Is there a reason for that?
This would use a template now by default, so I guess we should convert that.
Comment #13
pribeh CreditAttribution: pribeh commentedI've tested the patch from #5 on the latest 7.12 and it works great.
Comment #14
pribeh CreditAttribution: pribeh commented@gregnostic Count no longer works after applying the patch in #5 for 7.x for me.
Comment #15
unfrgivn CreditAttribution: unfrgivn as a volunteer commentedThank you so much slashrsm for your initial work on this. I have updated your patch to work for Drupal 8.2.x
I didn't test this throughly but it applies successfully via composer now and it works on the front-end via button click. I had to replace the lazy load theme method with a twig template in the templates dir. You would need to modify or override this template in order to change the button look/text.
My apologies if this was not done "properly", I usually work on proprietary modules and so I haven't contributed a ton to Drupal.org
Comment #16
Chris Matthews CreditAttribution: Chris Matthews commentedHere is a new D8 patch that simply fixes the whitespace errors in the #15 patch
Comment #17
Chris Matthews CreditAttribution: Chris Matthews commentedComment #18
spfaffly CreditAttribution: spfaffly at VMLY&R commentedThanks @slashrsm and @chris-matthews
I piggy-backed off your patch to add Comment Count functionality to the "Show Comments" button.
Patch below.
Comment #19
spfaffly CreditAttribution: spfaffly at VMLY&R commentedPatch added.
Comment #20
spfaffly CreditAttribution: spfaffly at VMLY&R commentedForgot to add untracked template file. Rerolled patch. Also fixed issue with undefined variable.
Comment #21
spfaffly CreditAttribution: spfaffly at VMLY&R commentedPatch added.
Comment #22
spfaffly CreditAttribution: spfaffly at VMLY&R commentedComment #23
dawehnerCan someone document what this patch is solving in the issue summary? Why we are loading the comment count when we just want to lazy load JS files? Maybe I'm missing something though.
Comment #24
spfaffly CreditAttribution: spfaffly at VMLY&R commented@dawehner
My apologies - my work is built on top of the lazy load JS file functionality and wanted to provide the code here just in case.
If needed once the lazy load JS file functionality is merged I can reroll the files into a separate patch/issue.
Thanks!
Comment #25
jigariusThumbs up for this issue. Enabling the Disqus comments pulls down my site's page speed score from 90s all the way down to 50s.
Comment #26
jigariusI see that there's a patch for this problem, but that it doesn't use an IntersectionObserver. Most modern browsers support it and it will be worth it to take a look. I'll try the patch soon.
Here's how I'm solving the issue for my site:
1. Create a checkbox in Disqus settings saying whether to lazy load the Disqus thread
2. If lazy loading is enabled and if the browser does not support IntersectionObserver, there will be no lazy loading. It might be tempting to create our own intersection observer, but all modern browsers already support them or will eventually support them. So, it is better not to write additional code which will be deprecated soon.
3. If lazy loading is enabled and the browser supports IntersectionObserver, then attach an intersection observer which will load the Disqus thread when the user scrolls down to the comments area.
While we're working on the disqus.js file building this feature, we can also do some cleanup (optional). Like using $ sign instead of jQuery and breaking things into 2 behaviors, one for comment counts and one for the thread.
Comment #27
jigariusJust tested the patch in #22 and it works very well. Thanks! It would've been better to use an Intersection observer rather than a scroll observer. Also, there are many disqus_* variables in the global namespace (not created by the patch), it will be better to put them inside the Drupal.disqus namespace to keep the global namespace cleaner. This can be done as a separate task though.
The new lazyloading settings are not present in the config schema.
As for the comment counts, can we not simply detect if there is a comment-count related HTML element on the page? If some element needs a comment count, we load them, otherwise, we don't – one less setting to manage. For example, the thread is only required when the page contains a div#disqus_thread element. In the same way, the comment count should only be loaded when there are relevant elements present on the page.
Comment #28
spfaffly CreditAttribution: spfaffly at VMLY&R commentedComment #29
jigariusComment #30
jigariusThe patch in comment #22 has worked for me and it's deployed to my prod site. I'll let the maintainers take it from here. If you're a maintainer please see my previous comment number 27. At least the config schema needs to be updated – the rest is optional I guess. Kindly make sure you credit the right people.
Comment #31
renguer0 CreditAttribution: renguer0 as a volunteer commentedI installed the patch #22. The configuration page, button and comment counts works (plus twig template) but when I click in "Show comments" button, nothing happens. In configuration page, I setted up the default class on button that I saw in twig template.
Console of Chrome debug don't show any errors at all. What can I do?
EDIT: Enabling comments when scroll down works. Thanks for that!!
Comment #32
renguer0 CreditAttribution: renguer0 as a volunteer commentedComment #33
asrobUpdated patch for version 7.x.
Comment #34
dzepol CreditAttribution: dzepol commentedI tried to apply the patch from #33 but it says this hunk is rejected. The rejections occur whether or not I apply against the dev or the lastest version of 7.
Comment #35
jigariusSince Drupal Core has started an initiative to remove jQuery usage, I have started the initiative of removing jQuery dependency from visitor-facing scripts of the Disqus module.
I'd request you to help test those changes because after they're merged, I will help fix this issue without jQuery asap so that your pages load faster without jQuery and lazy-loaded Disqus comments :D
Comment #36
DamienMcKennaComment #37
asrobI've updated a patch to work with version 2.0.1-alpha1 because I've to use it with Drupal 9.
Comment #38
asrobI've updated a patch to work with version 2.0.1-alpha4.
Comment #39
gaurav.kapoor CreditAttribution: gaurav.kapoor at Axelerant commentedComment #40
DamienMcKennaThe patch doesn't apply against the 2.0.x branch.
Comment #41
chikePatch #38 broke the module on 2.0.1-alpha5. The comments stopped loading.
Comment #42
asrobUpdated patch to work with 2.0.1-alpha5.
Comment #43
gaurav.kapoor CreditAttribution: gaurav.kapoor at Axelerant commentedComment #44
chikeAfter applying patch #42, visiting configuration page throws a white screen with this error:
Class 'Drupal\disqus\Form\DisqusCommentManager' not found in Drupal\disqus\Form\DisqusSettings Form->buildForm() (line 293 of C:\wamp64\www\site\modules\contrib\disqus\src\Form\DisqusSettingsForm.php)
Comment #45
gaurav.kapoor CreditAttribution: gaurav.kapoor at Axelerant commentedComment #46
ckngLazy load via IntersectionObserver version. Changes and improvements
Comment #47
ckngBackported IntersectionObserver version to 1.x. No tests, since there is none setup for 1.x.
Comment #48
ckngComment #49
dpagini CreditAttribution: dpagini as a volunteer commentedThis is marked as for the 2.x branch, but there is no patch for that branch right now... should this go back to "needs work" then? I'm happy to test out a patch for 2.x, but I'm a little lost on what the patch is doing exactly.
Comment #50
dpagini CreditAttribution: dpagini as a volunteer commentedOh this is my bad. I did not read close enough that #47 was a backport for 1.x, but #46 already existed for 2.x. Once I took some time and read slowly, I pulled in that patch on my site, and tested it out, and my PageSpeed scores were significantly improved. Marking as RTBC.
Comment #51
IT-CruPatch #46 worked so far for me, but I see some possible improvements we could maybe also integrate.
if (settings.disqus.language || false) {
toif (settings.disqus.language) {
.var disqus_lazy_load = 0;
is never used.@ckng: what do you think about possible improvements? Should we maybe set this issue back to needs work?
I've also linked related issue where already in the past some work was done to remove jQuery from disqus.js.
Comment #52
jigarius@ 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?
Comment #53
IT-Cru@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 :)