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

Update events listeners for page specific scripts #1667

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

amitaibu
Copy link
Collaborator

@amitaibu amitaibu commented May 15, 2023

  • Remove ihp:load
  • Add comment about the recommend use of jQuery's on
  • Add an example on how to avoid double binding.

@amitaibu amitaibu marked this pull request as draft May 15, 2023 12:32
@amitaibu amitaibu marked this pull request as ready for review May 16, 2023 07:01
@@ -1,10 +1,3 @@
var ihpLoadEvent = new Event('ihp:load');
Copy link
Member

Choose a reason for hiding this comment

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

This change is a BC break, not sure whether that's worth it. ihpLoadEvent is also used somewhere in the file, so we'd need to remove the usages as well in case we want to delete these variables

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change is a BC break, not sure whether that's worth it

In my tests I saw that ihp:load was not working as expected. So on the one hand it's BC break, but on the other - people using it might think it's working. Should we ask on Slack who's using ihp:load, see if we get people to answer?

Copy link
Member

Choose a reason for hiding this comment

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

I just checked and saw we're ourselves using this in several projects:

image

e.g. like this:

image

So let's keep this event here. But I'm fine with removing the outdated documentation on this and replacing it

lib/IHP/static/helpers.js Outdated Show resolved Hide resolved
@amitaibu
Copy link
Collaborator Author

hmm, I'm able to reproduce on my repo a case with $(document).on('ready turbolinks:load', function () { not working. So let's not merge yet.

@amitaibu amitaibu marked this pull request as draft May 17, 2023 18:01
@amitaibu
Copy link
Collaborator Author

Here's a reproducible example where the custom on ready handler isn't invoked transitioning into the page.

amitaibu/ihp-landing-page#7

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.

None yet

2 participants