jquery-ui-month-picker icon indicating copy to clipboard operation
jquery-ui-month-picker copied to clipboard

Fix for jqueryUI 1.12

Open wallenium opened this issue 7 years ago • 20 comments

#72 Might not be the best implentation, but works. Hint: I changed the JS file to the unminified Version, it is easier to debug. Needs to be altered once you tested the fix.

It should work with < 1.12 and > 1.12, i added a $ui.version check.

wallenium avatar Mar 17 '17 08:03 wallenium

There are a few issues the need to be resolved before we can merge:

  • When you hover the "Year 2017" button it's not animating the buttons label to say "Jump Years".
  • When you click the button and then go back to the months view the next/prev buttons are disabled.
  • We should really find a more DRY way to handle the jQuery UI button options.

There seems to be an issue with the Travis-CI build in general. I'll fix it when I can find time to look at it.

benjamin-albert avatar Mar 20 '17 17:03 benjamin-albert

Will Check

No idea how to reduce repetitions withiout loosing bckward comp. If You have any idea..

wallenium avatar Mar 22 '17 23:03 wallenium

According to jQuery UI 1.12 Upgrade Guide:

Although the redesigns introduce breaking changes, 1.12 maintains a lot of compatibility with the 1.11 API by default.

However you can disable backwards compatibility by:

<script src="jquery.js"></script>
<script>$.uiBackCompat = false;</script>
<script src="jquery-ui.js"></script>

It seems they want to remove backwards compatibility in jQuery UI 1.13 so unfortunately I still think it's worth supporting both APIs.

The idea I have is to define the options once according to the new API and always pass them through a jQuery plugin we create which converts the options to the old API if necessary.

Example:

var _isNewApi = $.ui.version >= '1.12';

$.fn.monthPickerButton = function(opts) {
  if (_isNewApi) {
    return this.jqueryUIButton(opts);
  } else {
    var newOpts = {};
    if (opts.showLabel !== void 0) newOpts.text = opts.showLabel;
    // etc...

    return this.jqueryUIButton(newOpts);
  }
};

this._prevButton.monthPickerButton({
    showLabel: false
});

benjamin-albert avatar Mar 22 '17 23:03 benjamin-albert

I reworked the parts, fixed all bugs. Hopefully implementation is better now ;) Learned a bit, never used functions for These type of work. Thanks a lot

wallenium avatar Mar 23 '17 10:03 wallenium

The index.html file in the /demo directory is not intended for development, it's intended to show users how to setup and use the plugin (which is why it uses the minified version).

@wallenium Please revert the changes to use the source files in the demo and read the Contributing section of the project's Wiki.

benjamin-albert avatar Mar 23 '17 20:03 benjamin-albert

@wallenium It looks like the tests are filming please fix the test so we can merge the pull request.

benjamin-albert avatar Mar 23 '17 20:03 benjamin-albert

@wallenium Please reach out if you need help fixing the tests.

We can also send you an invite to join our slack room if you want.

benjamin-albert avatar Mar 23 '17 20:03 benjamin-albert

After pulling your branch and adjusting the test environment to use jQuery UI 1.12 it looks like there are a lot of visual glitches: screen shot 2017-03-23 at 2 16 41 pm

@wallenium Please run the tests from master to see how the plugin should look

benjamin-albert avatar Mar 23 '17 21:03 benjamin-albert

Gott possible to run the test on my system, not sure why it fails:

PhantomJS threw an error:ERROR

0 [ '' ] Warning: PhantomJS exited unexpectedly with exit code null. Use --force to continue.

latest its version available. Should work without a problem.. Checked the contribute page. Layout was a bit different but not that broken as in your screenshot during my tests. At least for me it was acceptable ;)

wallenium avatar Mar 23 '17 22:03 wallenium

I am also running into this issue on my work machine.

You will have to run the tests from the browser until I can get around to fixing that issue.

benjamin-albert avatar Mar 23 '17 22:03 benjamin-albert

@wallenium Don't forget to adjust the test.html file to use jQuery UI 1.12

benjamin-albert avatar Mar 23 '17 22:03 benjamin-albert

Some of the failing tests seems to be because of internal problems. Not sure why done() fails...

RTL is working, the test needs to be adjusted because of the missing span, not sure why the test result shows wrong dates with strange numbers while the manual tests on the same page are working fine..

only strange things seems to be the click behavior and the disable of prev/next button if limit is reached..

wallenium avatar Mar 23 '17 23:03 wallenium

@wallenium What do you mean by internal problems? Can you be more specific?

Which test are you having a problem fixing?

benjamin-albert avatar Mar 23 '17 23:03 benjamin-albert

When I finish writing support for jQuery 1.12 I will open a pull request which will include @wallenium commits.

I think once that PR gets merged this PR will be considered merged as well so I'm leaving it open for now.

@KidSysco Do you think we should keep this PR open unit we merge my PR or we should close this PR?

benjamin-albert avatar Apr 24 '17 04:04 benjamin-albert

Sorry, was a bit busy the last weeks. Hopefully i have the time this week to finalize this issue. Or should i wait for you, @benjamin-albert

wallenium avatar Apr 24 '17 07:04 wallenium

@wallenium I think before we implement support for both versions of the jQuery UI APIs in a single file I want to change our test suits to:

  1. Support testing both versions of the API with a "push of a button".
  2. Make contributing easier by making it possible to run the tests without installing Node.js.

After I adjust the test suite I'd be happy to help you write support for the new API or pick up writing support if you're not up for it.

@KidSysco Can you please send @wallenium an invite to our Slack team. I think this would make collaborating with him easier.

benjamin-albert avatar Apr 24 '17 19:04 benjamin-albert

Sorry I did not reply until now. Springtime chores got me all busy.

I am fine with leaving the PR open until it is ready to merge. It might result in a merge conflict but that should be OK. Either way, whatever is easiest for you guys.

I would be happy to send @wallenium an invite to our slack channel. I will need an email address from you @wallenium in order to do that.

The slack channel is nice for chatting.

Let me know if there is anything else I can do for use guys!

KidSysco avatar Apr 25 '17 18:04 KidSysco

@wallenium I've pushed changes to my jquery_1.12_support branch which allow you to test both jQuery UI API versions.

Just open test/test.html and click Run against jQuery UI 1.12.1 API.

@wallenium If you'd prefer that I write support for jQuery UI 1.12 then just say so, and I will be glad to do it whenever I can get around it (usually on the weekends).

benjamin-albert avatar May 01 '17 02:05 benjamin-albert

@wallenium I almost finished fixing the tests. I will start fixing the visual glitches when I can find time for it. (usually on the weekends).

Take a look at my jquery_1.12_support branch.

benjamin-albert avatar May 08 '17 04:05 benjamin-albert

A bit of an update I've been busy trying to fix the visual glitches and I came to a conclusion that I could not fix them.

Since then I've managed to figure out how to git in touch with the jQuery UI development team (which unfortunately isn't as straightforward as opening a GitHub issue) and I have been discussing the issues with @scottgonzalez.

For details discussion:

https://forum.jquery.com/topic/adjusting-the-jquery-ui-month-picker-plugin-for-jquery-ui-1-12

benjamin-albert avatar Jun 05 '17 17:06 benjamin-albert