-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Adds ability to fire Trigger listeners immediately. Fixes #10869 #10877
Conversation
Hi @kentontaylor, Thank you for your PR, I'll review it tonigh. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kentontaylor !
Your PR works, but seems a bit hacky. Foundation should just try to initialize himself as soon as possible. A iDontCareInitializeNow()
function would only fails in most of its use cases.
As far as I know, there is no reason to wait for all scripts to be loaded. When html is dynamically added we expect it to be initialized manually anyway.
I replaced $(window).on('load')
by $(document).ready()
and it seems to work. Unit tests runs and all components are fine. However, I'm not totally confident with this.
@colin-marshall @Owlbertz @brettsmason Do any of you know Trigger
enough to ensure that using $(document).ready()
does not introduce weird bugs ?
Hi @ncoden, thanks for the feedback! Yes, we noticed too that swapping So instead, my goal became introducing the capability we needed as optional, while avoiding possible regressions in downstream components/behaviors that may have come to rely on the timing of Looking forward to the others' input. Thanks again! |
I really don't want to sound rude, but the broken unittests indicate an issue to me. |
@Owlbertz The broken unittests comes from elsewhere. I did not figured out from where, they works in local. |
Closes #10869 |
@ncoden I guess this is another candidate for the new |
So we may have to use |
No description provided.