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

knockout-jqueryui 2.2.3 not fully compatible with Jquery UI 1.12 (buttons break) #61

Open
Hiswe opened this issue Nov 30, 2016 · 8 comments

Comments

@Hiswe
Copy link

Hiswe commented Nov 30, 2016

jquery-ui 1.12 introduce a new API for buttons and knockout-jqueryui is still based on the old one.

@bago
Copy link
Collaborator

bago commented Nov 30, 2016

Can you describe what is "not fully compatible"?
An use case, a test case, error messages... a jsbin/jsfiddle to reproduce the issue...

@Hiswe
Copy link
Author

Hiswe commented Nov 30, 2016

@bago It's mostly some icon only buttons being converted to icon + text

What it should look like (gallery icon + cancel/confirm icons):

screen shot 2016-11-30 at 16 33 22

After the update: gallery is now textual, cancel/confirm show their label (the buttons overflow is not related to this of course):

screen shot 2016-11-30 at 16 31 38

For the test case I really don't know how to make a good one for now…

But as I have read from jquery-ui doc:

{
  classes: {
    "ui-button": "highlight",
  },
  disabled: true,
  icon: { 
    icon: "ui-icon-gear",
    iconPositon: "end",
  },
  label: "custom label",
  showLabel: false,
}

instead of (from button.js): this.options = ['disabled', 'icons', 'label', 'text']

I hope it helps a little bit ^^

@bago
Copy link
Collaborator

bago commented Dec 1, 2016

The jqueryui 1.12 upgrade guides says that the old options are "DEPRECATED" BUT they should works in a backward compatible way unless specifically disabled:
https://jqueryui.com/upgrade-guide/1.12/#deprecated-support-for-checkbox-and-radio-types-in-favor-of-checkboxradio-widget

So, this sounds a bug in jQueryUI 1.12 :-(

@Hiswe
Copy link
Author

Hiswe commented Dec 1, 2016

I should come up with a pull request. Looking at the codebase this shouldn't take long.
But It will break old jquery-ui versions support for this particular component

@bago
Copy link
Collaborator

bago commented Dec 1, 2016

I opened 2 issues against jQuery UI 1.12

https://bugs.jqueryui.com/ticket/15108
https://bugs.jqueryui.com/ticket/15109

Now, docs says that jQuery UI 1.12 should be backward compatible with 1.11, but it clearly is not so we probably should simply declare knockout-jqueryui compatible with jquery ui 1.11 and tell people to avoid using the buggy 1.12.

Also, 1.12 introduces new API that will be the only API available in 1.13.
Using the "new API" doesn't only means altering the options array, but also changing the html markup.. so I don't know what is the best option. Maybe knockout-jqueryui should simply stick to 1.11.

Creating a knockout-jqueryui release compatible with the new API is "out of scope" for me, so unless anyone one else is ready to take the big issue, we'll probably have to work on the documentation side to let people know jQueryUI 1.12 issues and jQueryUI 1.13 breaking changes. I really don't like the jQuery idea to break API compatibility in a 1.12 to 1.13 release (well, they really broke it in 1.11 to 1.12 but this was not by purpose).

@Hiswe
Copy link
Author

Hiswe commented Dec 1, 2016

@bago Seems fair to stick to 1.11

And yes that's a good demonstration of how following semver convention could prevent those problems!

I won't close the issue… people may look in for informations :)

bago added a commit that referenced this issue Dec 1, 2016
jQueryUI 1.13 will change many APIs breaking ko-jqui.
jQueryUI 1.12 *should* simply deprecate some of them while retaining compatibility, but it failed doing that, so we can't depend on it for some component (see: #61 )
@bago
Copy link
Collaborator

bago commented Dec 1, 2016

I updated the bower declaration, the readme and the index page for the website so to include explicit reference to the jquery ui 1.8.x to 1.11.x as compatible.

@Hiswe
Copy link
Author

Hiswe commented Dec 1, 2016

@bago Thanks a lot!

ghost pushed a commit to firedrum-marketing/knockout-jqueryui that referenced this issue Apr 4, 2017
Add custom.combobox widget from jquery-ui.com Autocomplete example.
Add CommonJS support.

Signed-off-by: Jonathan Horowitz <jhorowitz@firedrum.com>
ghost pushed a commit to firedrum-marketing/knockout-jqueryui that referenced this issue Apr 6, 2017
Add custom.combobox widget from jquery-ui.com Autocomplete example.
Add CommonJS support.

Signed-off-by: Jonathan Horowitz <jhorowitz@firedrum.com>
ghost pushed a commit to firedrum-marketing/knockout-jqueryui that referenced this issue Apr 6, 2017
Add custom.combobox widget from jquery-ui.com Autocomplete example.
Add CommonJS support.

Signed-off-by: Jonathan Horowitz <jhorowitz@firedrum.com>
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

No branches or pull requests

2 participants