-
-
Notifications
You must be signed in to change notification settings - Fork 114
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
Fixes #9: widgets do not work after pjax load #23
Fixes #9: widgets do not work after pjax load #23
Conversation
@@ -75,7 +75,7 @@ protected function registerClientOptions($name, $id) | |||
{ | |||
if ($this->clientOptions !== false) { | |||
$options = empty($this->clientOptions) ? '' : Json::htmlEncode($this->clientOptions); | |||
$js = "jQuery('#$id').$name($options);"; | |||
$js = "jQuery(document).on('ready pjax:success', function() { jQuery('#$id').$name($options); });"; |
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.
That means widget has to use ready
in order to be initialised, right?
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.
@samdark Thanks for the second set of eyes - would like your opinion on a few things. First, to answer your question, the ready portion matches the current behavior, and the pjax:success adds the pjax support.
Taking a longer look at the code that's output, however, got me to thinking that the following may be a better approach?
$js = "jQuery(document).on('ready pjax:success', function() { jQuery('#$id').$name($options); });";
$view = $this->getView();
$view->registerJs($js, $view::POS_END);
This would pull the JavaScript into a standalone end-of-body segment instead of adding it to the existing jquery(document).on('ready') segment. Either way seems to be functionally equivalent, but this new snippet may be cleaner.
Let me know your thoughts and I can adjust as necessary. Thanks!
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.
Yes, I think separate block is a bit better. Ready inside ready is a bit weird.
Move jquery to standalone, not inside the existing ready block
@samdark Any concerns with the latest commit? |
Yes. I don't see why this separation is needed. |
@samdark It's to get around the inability to do $this->getView()::POS_END, and operates on the assumption that the code should avoid static access to the \yii\web\View class if possible. If that's not the case and accessing it is acceptable, that works too. If avoiding the direct reference is preferred, I thought the checked-in solution was a little cleaner than some of the alternatives that I know. If there's another way or you prefer either of these, let me know and I can modify. Thanks! $this->getView()->registerJs($js, constant($this->getView()->className().'::POS_END')); $this->getView()->registerJs($js, constant(get_class($this->getView()).'::POS_END')); |
Just do |
@samdark Thanks for the help on this one - let me know if there's anything else I can improve. |
…gets-do-not-work-after-pjax-load # Conflicts: # CHANGELOG.md
@samdark Just wanted to check in on this fix - any chance of inclusion in the next version, or is there anything else you'd like to see? Thanks! |
If you do not assign a unique id for each Jquery UI widget, this PR will likely break a page loaded by PJAX because default widgets enumeration ( We should remember, that PJAX request must provide all necessary HTML and JS by itself. With the latest updates in PJAX plugin (especially this one and this one), I don't think that this PR can not be accepted. Sorry |
Add the pjax:success in addition to ready so the widget is initialized after pjax load as well as initial page load