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

Add "fools protection" in BootstrapWidgetTrait #54

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

Conversation

WinterSilence
Copy link
Contributor

@WinterSilence WinterSilence commented Oct 13, 2022

Q A
Is bugfix? ✔️/❌
New feature?
Breaks BC?
Fixed issues #49

@WinterSilence WinterSilence marked this pull request as ready for review October 13, 2022 12:30
@WinterSilence WinterSilence changed the title Add fools protection to BootstrapWidgetTrait Add "fools protection" in BootstrapWidgetTrait Oct 13, 2022
@simialbi simialbi added severity:minor status:code review The pull request needs review. type:feature New feature status:need more info and removed status:code review The pull request needs review. labels Oct 13, 2022
@simialbi
Copy link
Collaborator

Doesn't this make debugging more complicated? If the element is not present, you don't see an exception in browser console any more with this change.

@WinterSilence
Copy link
Contributor Author

@simialbi I expressed my opinion in #49, but I think that he is not alone... I don't planed break popular extensions :)

@simialbi
Copy link
Collaborator

Ok. So kartik's gridview uses the widget without rendering the button? Maybe there is a better solution which does not break debug option 🤔.

@WinterSilence
Copy link
Contributor Author

@simialbi his widget using template like as $template = "{head}{body}{footer}"; and renders blocks for all placeholders, but user can customize template and remove some placeholders: $template = "{head}{body}"; in this case {footer} block still rendered. Footer output BS widget and widget add self JS to page.

@WinterSilence
Copy link
Contributor Author

@simialbi my solution is use preg_replace_callback('/{\w+}/', $callback, $template) instead strtr($template, ['head' => $this->renderHead(), ...]), but I don't plan to seminars "How optimize your code" for authors of popular extensions (:

@kartik-v
Copy link

kartik-v commented Mar 9, 2023

Doesn't this make debugging more complicated? If the element is not present, you don't see an exception in browser console any more with this change.

This is a breaking change from v2.0.3. When using earlier with jQuery in yii2-bootstrap5 v2.0.3 - no debug/error message was thrown even if the selector $id does not exist because of the way jQuery inherently works and been designed.

"jQuery('#$id').$name($options);"  // will not throw any console error even if selector with $id does not exist

I assume the same behavior should be implemented for use without jQuery - else its a BC Breaking change from 2.0.3 to 2.0.4.

var bs5pluginElement = document.getElementById('$id'); 
// this will throw an error when bs5pluginElement is used in subsequent javascript operations
// without validating if it is undefined

IMO... individual developers should choose to validate if needed for their use case, whether the element exists or not before initializing the plugin or bootstrap widget and the widget should be allowed to render like earlier without any errors - or this could be a configuration option for the widget/plugin.

@kartik-v
Copy link

kartik-v commented Mar 9, 2023

@simialbi my solution is use preg_replace_callback('/{\w+}/', $callback, $template) instead strtr($template, ['head' => $this->renderHead(), ...]), but I don't plan to seminars "How optimize your code" for authors of popular extensions (:

You are free to suggest any improvement to any extension code (if you want to) as its a OPEN SOURCE community where you can raise enhancement suggestions - just like you have raised a PR here. Have given few of my feedback for this PR as well. Thanks.

@newadventure079
Copy link

newadventure079 commented Jul 24, 2023

@WinterSilence Did you misspell lenght?

Also, bs5pluginElement.length > 0 will fail with Cannot read properties of null (reading 'length'). I changed it locally to bs5pluginElement != null && bs5pluginElement.length > 0

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

Successfully merging this pull request may close these issues.

4 participants