write unit tests #277
write unit tests #277
Conversation
| /* | ||
| * store original global keys | ||
| */ | ||
|
|
There was a problem hiding this comment.
minor: there should be no empty line between the comments and the first line of code that the comment refers to. This way automated tools like IDEs can associate them.
Some examples:
| // Initialize multiDatesPicker if it's not already defined | ||
| if (!$.ui.multiDatesPicker) { | ||
| $.ui.multiDatesPicker = { version: '1.6.6' }; | ||
| } |
There was a problem hiding this comment.
Aren't the lines above (22-24) equivalent to the one below (27)?
I would just leave one, and the latter would take precedence for the sake of reducing the amount of changes when not needed.
| var mdp_settings = { | ||
| dateFormat: 'mm/dd/yy' // Set default format | ||
| } | ||
|
|
||
| if(options) { | ||
| $.extend(mdp_settings, options); | ||
| } | ||
|
|
||
| $this.datepicker(mdp_settings); |
There was a problem hiding this comment.
I haven't tried it yet, but this default could override any default already present in the jQuery UI logic. It is a potentially breaking change, I would avoid adding this extra logic and just take whatever jQuery UI uses.
|
|
||
| if(end < begin) { // Swap if end is before begin | ||
| var temp = end; | ||
| end = begin; | ||
| begin = temp; |
There was a problem hiding this comment.
This is actually more readable and thus maintainable, thanks.
| end = begin; | ||
| begin = temp; | ||
| } | ||
| for(var i = begin; i <= end; i++) |
There was a problem hiding this comment.
This small change in the code is a big difference in behaviour. While some may expect the end date to be included, and it is valid, the current logic is excluding it, so this would be a breaking change for anyone already adopting this solution.
| $(this).removeData('multiDatesPicker') | ||
| .off('.multiDatesPicker'); | ||
|
|
||
| $(this).datepicker('destroy'); | ||
|
|
||
| var index = $.inArray(this, $.ui.multiDatesPicker.instances); | ||
| if (index > -1) { | ||
| $.ui.multiDatesPicker.instances.splice(index, 1); | ||
| } |
There was a problem hiding this comment.
I like the extra cleaning, thanks. I wouldn't be surprised to find out that this solves some of the open issues.
| $(this).removeData('multiDatesPicker') | ||
| .off('.multiDatesPicker'); | ||
|
|
||
| $(this).datepicker('destroy'); | ||
|
|
||
| var index = $.inArray(this, $.ui.multiDatesPicker.instances); | ||
| if (index > -1) { | ||
| $.ui.multiDatesPicker.instances.splice(index, 1); | ||
| } |
There was a problem hiding this comment.
Interesting extra steps. Where did you find the docs that suggested them? Are they actually needed in this context?
| const $ = require('jquery'); | ||
| require('jquery-ui/ui/widgets/datepicker'); | ||
|
|
||
| if (!$.ui) { | ||
| $.ui = {}; | ||
| } | ||
|
|
||
| if (!$.ui.multiDatesPicker) { | ||
| $.ui.multiDatesPicker = { version: '1.6.6' }; | ||
| } | ||
|
|
||
| require('../jquery-ui.multidatespicker.js'); |
There was a problem hiding this comment.
This is not needed. Any needed initialization is already speficied in jest.setup.js
| @@ -0,0 +1,5 @@ | |||
| const $ = require('jquery'); | |||
| global.$ = global.jQuery = $; | |||
|
|
|||
This is a modified version of the original pull request: #277 Pulled branch with fixes: https://github.com/dubrox/Multiple-Dates-Picker-for-jQuery-UI/tree/Moise1-unit-tests Reviewer: dubrox
|
I'm closing this pull request as I already added the needed fixes and merged with them to the main repo by keeping your authorship on the final commit: |
No description provided.