trevorcampbell / website_diff

MIT License
2 stars 1 forks source link

Handling pages with no jquery, removing highlights, waiting for page load #18

Closed trevorcampbell closed 2 months ago

trevorcampbell commented 3 months ago

Re #15 : @briank-git do you think we can reimplement website_diff.js without using jQuery? Just with base javascript, to avoid having to deal with dependencies? Or maybe you have a better idea how to handle dependencies

It would also be good if we solved #21 here

I think my solution for #14 , #17 , and #20 should be fine already

briank-git commented 2 months ago

Thanks @trevorcampbell for putting in the fixes.

The fixes for #17 and #20 look good.

For #15, I think adding the following to the start of website_diff.js as you did in commit f60a603 is sufficient because it will check if jQuery was already loaded before loading jQuery:

if (typeof jQuery == 'undefined') {
  document.write('<script src="https://ajax.googleapis.com/ajax/libs/jquery/3.7.1/jquery.min.js"></script>');
}

Also I think 3da2763 is redundant as the above snippet will load jQuery already.

But while testing above with a page that did not load jQuery natively, I found the page doesn't wait for jQuery to fully load before running the next function which used jQuery causing an error to be thrown about $ being undefined.

So instead of using $(window).on("load, function() ... on line 8 in website_diff.js, I found

document.onreadystatechange = function () {
  if (document.readyState == "complete") {
    // Function calls for scrolling to first diff and next diff
  }
}

allows for jQuery to fully load and should also cover issue #14.

If you think this makes sense then I can put my changes to the fixes for #15 and #14 in new commits to your branch.

trevorcampbell commented 2 months ago

@briank-git if you've thoroughly tested your idea and it works on large sites with and without jquery, then I'm happy with your proposal

trevorcampbell commented 2 months ago

One question: is the if statement in your proposed code snippet always guaranteed to trigger when a page loads (and is fully done loading)?

briank-git commented 2 months ago

@trevorcampbell From the documentation, it looks like document.readyState is "complete" right before the "load" event is fired (which I believe is when $(window).on("load", function() ... would run). https://developer.mozilla.org/en-US/docs/Web/API/Document/readyState#complete

To avoid the if statement, a better option could be to use

window.addEventListener('load', (event) => {
  // Function calls for scrolling to first diff and next diff
});
trevorcampbell commented 2 months ago

That looks better to me (again, assuming you've tested it on one of our larger sites and it solves the scroll-before-load issue)

briank-git commented 2 months ago

I tested my changes and they are working fine. Feel free to comment or merge if ok.