readyToBind fix

[coni2k]

Using boolean for readyToBind wasn't enough for reload, since watch
doesn't get triggered if "loaded = true;" was set on angular's
routeChangeSuccess event (probably because the value actually doesn't
change).

Therefore 'isReady & boolean' was replaced with 'loadedOn / DateTime'
and since on each call there is a new value, readyToBind gets triggered.

[coni2k]

Michael hi,
About this case, if you wish I can create a test project to show why it was needed.

[michaelbromley]

Hi,

Thanks for the PR. Yes, I think it would help if you could provide more context in the form of code that does not work with the current implementation. Also could you tell me if this change will change the public API in any way? It looks to me like you could now pass any value to ready-to-bind now in order to trigger a load. Is that right?

[coni2k]

Michael hi,

When I check it again, I managed to handle the case without modifying your file.

Please check these two pages;

1. Uses boolean value
   http://dev.forcrowd.org/dirDisqusFix/default_boolean.aspx
2. Uses datetime value
   http://dev.forcrowd.org/dirDisqusFix/default_datetime.aspx

dirDisqus.js file is your original file, except there is an extra console.log at line 54;
console.log('dirDisqus.js - isReady: ' + isReady);

In both cases navigate between views, Home, View1, View2 and check the console logs.

First case (default_boolean.aspx) uses disqusLoaded boolean. In main.js check line 19 - 20;
$scope.disqusLoaded = true;
As you can see, in this case, isReady is not triggered when view changes, since it keeps getting ('null' and) 'true' value.

Second case uses disqusLoadedOn, in main.js check line 24;
$scope.disqusLoadedOn = new Date();
new Date creates a new value in each call and this time scope.$watch for "readyToBind" gets triggered.

In your README page, you suggest using boolean values when the content is ready, which is actually correct in one time/async content case.
However, for single page application case, comments should be updated in every view change (by using routeChangeSuccess event) and in this case, we should keep changing the value, so passing booleans is not enough (incremental numbers could work as well, instead of new Date()).

So, what I would suggest is to update your README file by adding something like this - and close this PR without merging;

// simple example of controller with route changes
function myController($scope, $http) {
    $scope.disqusLoaded = null;

    $scope.$on('$routeChangeSuccess', function (event, current, previous) {
        $scope.disqusId = 'dirDisqus' + $location.path().replace(/\//g, '_'); // Your unique disqusId
        $scope.disqusUrl = $location.absUrl().substring(0, $location.absUrl().length - $location.url().length + $location.path().length);  // Your unique disqusUrl
        $scope.disqusLoaded = new Date();
    });
}

However, I believe, it would be a much better approach if there would be a watch on "disqusId" itself and update the comments, instead of having this extra "readyToBind".

If there is a value for disqusId (disqusId !== ''), load or refresh the related comments, else, don't load disqus. Covers all 3 cases (sync, async comments, routeChanges).
I'm pretty busy actually but if you think that's a good idea and can be merged, I can have a look and see how quickly I can do that.
Also with such update "readyToBind" would be obsolete, and removing it would be a breaking change, just a reminder.

And last, in any case, it would be great if you could remove or comment out this line from dirDisqus.js.
console.log('remote' + scope.disqus_remote_auth_s3);

Kind regards,

PS: Now when I tried opening my links (default.aspx pages) with Firefox, it kept loading them until it fails. In Chrome and Edge, it looks good. Let me know if you have an issue with loading the pages.

[michaelbromley]

Hi,

Thanks for the detailed response.

I agree with all your proposals. In this PR, you also fixed a number of clumsy style issues (as well as the console.log - duh! :) ). If you have time to implement what you propose that would be great. I am also super short on free time at the moment, and in the little time I have for OSS projects, I am working on an Angular2 pagination port.

I am fine with making a breakingchange to the API (especially as it is simpler) since I am using sermver versioning on the bower version, so people can avoid it if they want. Also, I don't think this module is very widely used anyway as far as I am aware (compared to, say the pagination one).