Skip to content

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

@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-cms-starter#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.

2 participants