Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix jQuery deprecation issue #447

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vilius
Copy link

@vilius vilius commented Aug 16, 2017

Uses $() syntax instead $(document).on("ready") which removes deprecation warning as of jQuery 1.8 and fixes the script for jQuery 3, where the method is removed.

See:
https://jquery.com/upgrade-guide/3.0/#breaking-change-on-quot-ready-quot-fn-removed
https://api.jquery.com/ready/#entry-longdesc

});
}
$(hideRemoveFields); // On page load
$(document).on('page:load turbolinks:load', hideRemoveFields); // On custom load events

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmmmm. I think this is not correct and I do not have a simple way to fix it. The "ready page:load" events are both replaced by the "turbolinks:load" event. So this works perfectly fine for backwards compatibility (if you use the new turbolinks). Indeed, if someone does not use turbolinks it will no longer be triggered. But someone using turbolinks 5 will, with your code, call hideRemoveFields twice.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nathanvda

I'm not sure if I understood why it should not work, but I don't have much experience with Turbolinks. To my understanding:

$(hideRemoveFields); // On page load

^ This will get executed on page load once regardless if turbolinks is used or not.

$(document).on('page:load turbolinks:load', hideRemoveFields); // On custom load events

^ This will get executed only when Turbolinks are used (either old version or the new one). It will get executed on initial load and subsequent turbolink-loads of body content.

Thus when Turbolinks is used the hideRemoveFields code will get executed twice on initial load. However it should not have any side effects.

Actually maybe this could be even better implementation:

  $(function() {
    hideRemoveFields();
    $(document).on('page:load turbolinks:load', hideRemoveFields); // Turbolinks support
  });

This will work with all jQuery versions. It will also work with Turbolinks and the code will be executed once per page load (either initial or one initiated by Turbolinks).

Am I missing something?

@nathanvda
Copy link
Owner

Thank you for your work (and fixing the documentation 👍 👏 👏). But if possible, I prefer to maintain backward compatibility. So not sure what the best approach is to achieve that, and if it is possible.

@nathanvda
Copy link
Owner

Hi there: I would want to merge this, but you have not yet updated the code as you mentioned in your last comment, which looks just fine (sorry I forgot to reply back to you earlier).

Also, with regards to the documentation you are confused: the synthetic event ready is deprecated, but not the ready() call, it is still supported, see https://api.jquery.com/ready/ so the README is correct (and I prefer the more verbose version, so the documentation as it is now). So I would love to merge this if it only contained the code-change as discussed before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants