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

Clean method invocation #1458

Merged
merged 3 commits into from Feb 12, 2016

Conversation

neocotic
Copy link
Contributor

This plugin has the widest and most configurable API that I have seen for a date picker widget and the documentation is fantastic, simple, and clear. Thank you!

That said; I noticed a slight abnormality with how to access this API. It's become almost a standard for top jQuery plugins to expose their API via the plugin method itself. For example;

// initialize plugin with default options
$(element).datetimepicker()
// initialize plugin with custom options on top of the defaults
$(element).datetimepicker({ locale: 'fr' })

// get a value
$(element).datetimepicker('date')

// set a value
$(element).datetimepicker('date', moment())

// perform a single action
$(element).datetimepicker('toggle')

// perform a chain of actions
$(element)
    .datetimepicker('show')
    .datetimepicker('date', moment())
    .datetimepicker('destroy')

This PR aims to add such support to this plugin in a non-breaking way (existing pattern will still work) and that also has minimum overhead.

This has been achieved by returning the return value of the called method unless it's a picker, which I'm treating as void and therefore returning the jQuery object to support chaining. I noticed that 4 methods did not return anything (really void), so there's a small workaround for these edge cases.

Caveats

As with every approach, there are some caveats, and I'd like to highlight these up front. These are generally considered the "norm", I believe, but I'll like to call them out anyway:

  • When a getter method is called when multiple elements are select, only the result of the last selected element will be returned
  • When attempting to call a method on an element where the plugin has not been initialized previously, an error is thrown (regardless of whether it was the only element selected or one of many)

Testing

I want to call this out immediately; I have not updated any tests. Yet! I wanted to get this PR opened early to see if there was a desire for this feature before spending time on the tests. I also hoped to get input on how the testing should be done as I see a couple of options for this:

  1. Add tests that only cover the changes that I've made
  2. Add tests for the new error cases (i.e. method called when plugin not initialized, jQuery plugin method passed first arg with unexpected data type) and then change all existing tests to use the new pattern for full test coverage

Obviously, the first option would be the smallest and have the least impact. The second option would be if you wanted to switch to the new pattern as your primary recommended pattern, which I be no means wish to push on anyone, but you may want to do so, so I'm just putting it out there.

For the same reason, I have also not updated any documentation or examples etc.

To clarify; I will write the tests before this PR is accepted, I just wanted guidance on how best to approach the tests.


Thoughts?

@Eonasdan
Copy link
Owner

Thank you for the kind words.

I remember looking up the jquery plugin method before and I thought at the time that it was preferred to do it the way that it's currently being done.

Personally, I prefer it the .datetimepicker() method!

Let's work on getting this PR in and maybe in v5 pull out the old way.

@neocotic
Copy link
Contributor Author

OK, I'll update the unit tests tomorrow night based on the first option and then, as you suggest, we can move right over to the new pattern entirely with the next major version. Good idea.

The benefit is that it won't ever actually break the existing pattern, since they can always access the plugin data. This isn't an issue and, in fact, it is the norm for plugins that use the new pattern. So it will mostly be a change to documentation and a refactoring of the tests and any examples to promote the preferred usage.

…s method to return option when no args are provided
@neocotic
Copy link
Contributor Author

I had some more spare time tonight than I thought so I've committed the changes to add the necessary tests.

I also spotted that the keyBinds method was broken since it didn't return the keyBinds option when no arguments were provided, so I fixed this at the same time since it's sort of related to this PR.

Let me know if you want me to make any changes.

@@ -299,6 +430,18 @@ describe('Public API method tests', function () {
expect(dtp.daysOfWeekDisabled).toBeDefined();
});
});

describe('access', function () {
xit('gets days of week disabled', function () {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There appears to be a separate issue with this function in that it doesn't work unless you explicitly set it to an array before hand, so I've raised #1459 to track this.

Eonasdan added a commit that referenced this pull request Feb 12, 2016
@Eonasdan Eonasdan merged commit 640e352 into Eonasdan:development Feb 12, 2016
Repository owner locked and limited conversation to collaborators Jun 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants