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

jQuery-UI-slider still in conflict #575

Closed
tlartaud opened this issue May 18, 2016 · 14 comments
Closed

jQuery-UI-slider still in conflict #575

tlartaud opened this issue May 18, 2016 · 14 comments

Comments

@tlartaud
Copy link

tlartaud commented May 18, 2016

See this comment : #443 (comment)
(wont give any fiddle, sry)

Regards.

@tlartaud
Copy link
Author

tlartaud commented May 18, 2016

Actually my fix is to load this block of code just before your script inclusion

<script type="text/javascript">
    <!--
    (function ($) {
        "use strict";

        // Fix for seiyria bootstrap slider conflicting with UI-Slider
        $.fn.slider = function () { return true; };

    })(jQuery);
    //-->
</script>

For others users, just add this block of code in your page before including bootstrap-slider script and you won't get any problem with ui-slider, and ui-slider will keep working.

Best regards.

@seiyria
Copy link
Owner

seiyria commented May 18, 2016

Feel free to submit a PR if it's of high importance.

@tlartaud
Copy link
Author

tlartaud commented May 18, 2016

@seiyria No thanks. Have a lot of work to do, spent a lot of times to debug and report. Feel free to fix your software by yourself. You have the whole solution explained right there. Now, you know why it's still conflicting for some users. The slider namespace should simply be changed for something less common or the alias binded in any case.

@poushy
Copy link

poushy commented Jun 7, 2016

@tlartaud Thank you for this work around.
Same issue happens if bootstrap slider is loaded from the different workflows, if in one of them jquery ui is present, in other it is not. It is prone to unseen issues, as in one workflow it will initialize as slider and in other as bootstrapSlider.

@caschbre
Copy link

@tlartaud Thanks for finding this. With your solution above does it break the other libraries using .slider() ?

@rovolution
Copy link
Collaborator

@caschbre @tlartaud

Would it help to have the ability to define your own alternate namespace?

This is our default behavior, but it seems like the ability to choose the namespace would be valuable? Let me know and I will add it to our "TODO" list.

Thanks!

@tlartaud
Copy link
Author

tlartaud commented Jul 14, 2016

@rovolution Hi. Yes that should help... But the really easier solution for everybody should be for you to only bind the namespace : bootstrapSlider, because depending on the environment where you deploy (on wordpress frontend vs wordpress backend), you won't get the same librairies and the same inclusions orders. Thats why, sometimes, slider will be the one from jquery-UI, sometimes not.
I'm really not in favor of such commons names like slider.. Because of standards namespacing behaviors, plus, this name is a bit unpersonnal.

I know your default behavior, but its wrong. Its a bad idea to do this.
Just try to create a test page to see the problem in action, with the following scripts included :

Index.html example 1

  • loading jquery
  • loading bootstrapslider.js
  • loading jquery-ui-slider.js
  • call .slider();

bootstrapslider is here loaded before jquery-ui-slider, and the function slider defined by bootstrapslider will be overrided by jquery-ui-slider. So, in this case, bootstrapslider become completely unavailable, you can't use it.

Index.html example 2

  • loading jquery
  • loading jquery-ui-slider.js
  • loading bootstrapslider.js
  • call .slider();

bootstrapslider is here loaded after jquery-ui-slider, so the slider function already exists. Bootstrapslider is in this case binding the function bootstrapSlider instead of slider.

Your idea about giving us the ability to define our alternate namespace is not bad, BUT, it won't fix the first example out of the box. Developpers will still have to understand why, for some reasons, the slider function doesn't return the same thing in some pages, and then, define an alternate namespace. The really best way is to bind a complex namespace by default (bootstrapSlider is okay), and, optionnally, also give the ability to bind an alternative custom namespace.

@caschbre I did not test that fix with all librairies, but with jquery-ui-slider, yes, both librairies will keep working with slider() for jQuery-UI-slider, and bootstrapSlider() for the bootstrap slider script.

@seiyria Sounds easy for you to thumb down someone reporting you an issue without a PR. Maybe you can first discuss the problem instead of just asking for a PR that you would probably not accept. The problem for me was fixed by my workaround, but i wanted to help you understand the problem by reporting it. Even if its not a PR, im actually contributing to help solving an issue that a lot of people should encounter. Maybe that I should just stop writing anything here, and stop reporting, because apparently, this not what you expect from your users. Good luck to you.

@rovolution
Copy link
Collaborator

rovolution commented Jul 14, 2016

@tlartaud makes sense. Thanks for reporting the issue.

I am always somewhat hesitant to publish breaking changes, so here is what we might do instead: have the slider always bind to the bootstrapSlider namespace and then only bind to the slider namespace if none currently exists.

That should address the order of loaded scripts issue described here yet not break the slider for any existing users of the slider namespace.

@tlartaud
Copy link
Author

tlartaud commented Jul 14, 2016

@rovolution yes, that's the correct thing to do. But you have to keep in mind that it won't fix the issue. Someone who's using your script with the slider namespace can still have conflicts, if, in some pages, jQuery-UI-slider is also loaded, because the slider namespace will be attributed to jQuery-UI-slider (so, for theses pages, he must use bootstrapSlider instead).

To prevent that, you should also recommend in your documentation to use the namespace bootstrapSlider instead of slider, when loading the script via jQuery, because this namespace is already used by some jQuery libs and could lead into unexpected problems.

Regards.

@rovolution
Copy link
Collaborator

Someone who's using your script with the slider namespace can still have conflicts, if, in some pages, jQuery-UI-slider is also loaded, because the slider namespace will be attributed to jQuery-UI-slider (so, for theses pages, he must use bootstrapSlider instead).

I am also going to add a console warning that appears indicating to the user that something is already bound to the slider namespace and that it is recommended that they use the longer namespace.

To prevent that, you should also recommend in your documentation to use the namespace bootstrapSlider instead of slider, when loading the script via jQuery, because this namespace is already used by some jQuery libs and could lead into unexpected problems.

We will definitely make sure to emphasize that in the docs. thanks!

@tlartaud
Copy link
Author

The console warning is really a great idea, and can help developpers to not losing time searching everywhere what is going on.

Thanks you to had accepted to discuss about it, and to find the best way to solve it ( I will definitely not say that to your mate @seiyria ... )

Have a nice day,
Cheers.

@seiyria
Copy link
Owner

seiyria commented Jul 14, 2016

I don't quite appreciate the snide remarks, as I've mostly held off commenting further in this issue. I still think it's quite simple: don't use two slider libraries. @rovolution is definitely being a great guy here and working towards a compromise, but we differ in opinion here.

@rovolution
Copy link
Collaborator

Locking this issue as I think its getting a bit off topic :)

Repository owner locked and limited conversation to collaborators Jul 14, 2016
@rovolution
Copy link
Collaborator

published to v9.1.0. see changelog for further details

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

No branches or pull requests

5 participants