This is a javascript multiselect calendar widget using the [MultiDatesPicker](http://multidatespickr.sourceforge.net/) library on top of the date_popup module.
It allows you to select multiple dates (with no time) in just one field, instead of having to add multiple popup widgets.
Link to sandbox page: https://www.drupal.org/sandbox/jackbravo/2417391
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/jackbravo/2417391.git date_multiselect
Parereview: http://pareview.sh/pareview/httpgitdrupalorgsandboxjackbravo2417391git
Most of the errors are on the multidatespickr.js file which is an external project that is included. The multidatespicker library is GPL.
Comment | File | Size | Author |
---|---|---|---|
#21 | Screenshot 2015-04-30 17.17.57.png | 84.21 KB | andrefy |
Comments
Comment #1
PA robot CreditAttribution: PA robot commentedWe are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)
Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).
I'm a robot and this is an automated message from Project Applications Scraper.
Comment #2
rivimey@jackbravo thanks for contributing.
The pareview checks are failing in a number of ways. Please fix all issues.
You have added a 3rd party library inline in this module, which is very discouraged : see https://www.drupal.org/node/422996 : "In general 3rd party libraries are forbidden". I don't see anything in the exceptions to that rule which would apply here.
I suggest you :
- include a hook_requirements implementation to check if the JS is in a ./libearies folder and complain if not. Many other modules do this...
- include instructions in the readme indicating where the JS is available and if there are any version restrictions.
Comment #3
rivimeyAutomated Review
Needs work
Manual Review
The readme file is lacking detail and lacks important sections.
The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.
If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.
This review uses the Project Application Review Template.
Comment #4
jackbravo CreditAttribution: jackbravo commentedThanks for the review! =).
About the 3rd party library I didn't give much thought about it. I based my code on the date_popup module code, which does the same for the http://plugins.jquery.com/project/timeEntry plugin: it is included with the date module. And I can think of other couple examples: colorbox, bootstrap, lightbox. Anyway, I can see that new modules do as what you suggest: chosen, jquery_ui_multiselect_widget. So now I use the same approach as the chosen module and we now need a manual download of the multidatespicker jquery plugin. README has been updated.
You added a secure code section, but I don't see how I'm not complying with the specifications. I use t() where a translation is needed, but I don't save any input to the database in my module but instead rely on other modules for that (date and the field API) so I don't think I need to use filter_css or check_plain.
What else would be needed? And thanks again for the review!
Comment #5
mlmoseley CreditAttribution: mlmoseley commentedMy understanding is that if you are using a third-party library, you need to make it a library off of sites/all, and access it that way. It doesn't matter what other projects have done or were permitted to do -- that is current best practice. You can create your own version of the the library that includes the multidatespicker jquery plugin and provide it with the module.
https://www.drupal.org/node/1342238
Comment #6
mlmoseley CreditAttribution: mlmoseley commentedAnd the git clone command above requires your password.
You need to post the the public git clone command. On your sandbox page select the 'version control' tab, then deselect 'maintainer', then click on the 'show' button, then copy and paste the git clone command.
Comment #7
jackbravo CreditAttribution: jackbravo commentedHi, I'm sorry. The version control tab on the sandbox seems to give me the private clone command. I signed out and copied the public instead.
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/jackbravo/2417391.git date_multiselect
And you are right, about not including the javascript library on my module I'm doing that now. The javascript file is not included and requires a manual download to the libraries folder, which is described in the README.txt file.
Comment #8
e.bogatyrev@jackbravo thank you for contributing.
There are some issues I found:
date_multiselect.js
date_multiselect.module
Comment #9
jackbravo CreditAttribution: jackbravo commentedThanks a lot @e.bogatyrev. I've fixed all the issues. Many where because of old stuff from the module this is based on that should be fixed as we eliminated unnecesary stuff (date_popup).
Comment #10
PA robot CreditAttribution: PA robot commentedProject 1: https://www.drupal.org/node/2417425
Project 2: https://www.drupal.org/node/2468435
As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).
If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.
I'm a robot and this is an automated message from Project Applications Scraper.
Comment #11
zbombicz CreditAttribution: zbombicz commentedHi jackbravo,
It will be a very helpful module. Unfortunately I found some mistakes, errors. Please check them out.
Automated Review
pareview:
FILE: /var/www/drupal-7-pareview/pareview_temp/js/date_multiselect.js
------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
------------------------------------------------------------------------
6 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 0
44 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 0
Manual Review
"The requirements section (required) shall make it clear whether this project requires anything outside of Drupal core to work (modules, libraries, etc)."
The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.
If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.
This review uses the Project Application Review Template.
Comment #12
jackbravo CreditAttribution: jackbravo commentedHi @zbombicz!
Thanks a lot for your review.
1. (*) Undefined index notices (minDate, maxDate,...). date_multiselect.module line 55, 61. I recommend adding default values to hook_field_widget_info.
Fixed.
2. (*) Misleading labels for fields Minimum and Maximum date relative to the current date. The label concludes, that I have to add dates, but in fact I need to add numbers of days. I recommend providing field description to explain operation.
We added a description to the field to explain how to use it.
3. (*) The module is not working if I choose collect end date. Also it is weird, that I can check Hours, minutes, seconds option, if its not treated. If these options are meaningless in your module, I recommend disable these options, or make extra validations for checking faulty configuration (in case of Multiselect calendar widget).
We implemented hook_date_field_settings_form_alter to alter the field form when using the date_multiselect widget type so we don't show the enddate field and set the granularity to ['year', 'month', 'day'].
4. (+) The provided (info file) configuration page admin/config/date/date_multiselect doesn't exists.
Fixed
5. Readme.txt: Add external multidatespicker library into Requirements section.
Fixed.
6. Typos: realtive
Fixed.
7. Add argument documantation for functions: date_multiselect_date_format, date_multiselect_implode_dates.
We removed this functions and instead we are now using a DATE_MULTISELECT_FORMAT constant. Since we don't show dates on the field to the user, the #date_format element configuration was not needed.
Comment #13
mvdve CreditAttribution: mvdve commentedSimple but very usefull module. Still some issues that need to be fixed.
First, pareview has found some small issues regarding coding standards.
See the pareview page.
Camelcase and underscores are used for variables. Within Drupal only underscores are used. See the Drupal naming convention.
A suggestion: When using external libraries, it is a plus to use the libraries API.
This gives you a very simple API to load and detect the library in sites/all/libraries.
Load and checking the library can be as simple as:
Comment #14
mvdve CreditAttribution: mvdve commentedComment #15
jackbravo CreditAttribution: jackbravo commentedOk, so I'm now using the libraries module to include the library. I also implemented a hook_requirements functions to let know the user if he doesn't have a correct version of the library.
I'm not sure what camelCase variables you are referring to. Could you provide a line number or function name? If you are talking about the names in the settings array in date_multiselect_element_process(). Those are camelCase because they will be translated into javascript variables, and I think this is the way it is used in other places (like date_popup).
I fixed one of the pareview issues. Two indenting issues remain, but I'm afraid that fixing them would mess up the version control information for that file (I would need to change almost all the lines in that file). So I think it is better to leave it like that. This error is carried over from date_popup, which was the base module for this one. Or what do you think?
Comment #16
rivimeyHi, Just re-run the pareview tests and it shows a bunch of issues on the .install file as well as the two indentation issues I think you refer to. I can cope with the JS indentation issue but reindenting a file shouldn't be a problem, really.
I suspect the camelCase that the other reviewer mentioned is in the names of the settings - for example, yearRange at 151, dateFormat, firstDay at lines 256 & 257, maxPicks at 265. As you have not consistently used camelCase for these (see for example year_range at 251), it would be better to be consistent and for better or worse the Drupal standard is for underscores. I am no JS expert though so would suggest seeking advice from someone like @klausi.
The new code for requirements looks reasonable, though the description attribute line must be broken up: the line is far too long. You should probably make it 5 or 6 lines, using intermediate variables. You may be able to extract common elements to some degree.
I'm concerned about the DATE_MULTISELECT_FORMAT being hard-coded to USA style; other countries don't use that style. If it must be hardcoded, then use the ISO standard (YYY-MM-DD) but much better to make it configurable.
It's good that you've considered a responsive design (JS file, line 16), but hardcoding 380 pixels for the window width is not that helpful; fixed numbers make assumptions. Could that be configurable, or a way found that makes it responsive without assuming fixed values?
Comment #17
jackbravo CreditAttribution: jackbravo commentedHi @rivimey, thanks for the review =)
You are right, I did messed up with the camel case and underscore syntax! I fixed the wrong year_range variables. According to drupal documentation seems like camelCase is preferred for JS: https://www.drupal.org/node/304258#drupal-settings
About the pareview. I fixed all the issues. Reindenting a file is an issue when you want to use git blame later to know who changed a lined, or on what commit it was changed. But luckily enough it is an issue common enough that seems like there are solutions:
- http://stackoverflow.com/questions/3945382/git-commit-that-doesnt-overri...
- http://makandracards.com/makandra/13245-git-blame-how-to-ignore-white-sp...
So I got ahead and fixed it.
We added DATE_MULTISELECT_FORMAT as a constant instead of a user defined variable like variable_get because in reality the user never sees it. He uses the widget instead. So I don't think it is necessary to change it.
About camelCase in JS. I found nothing specific in https://www.drupal.org/node/260140
And finally, we changed the hardcoded 380 value to a variable_get() call function.
What do you think?
Thanks again!
Comment #18
andrefy CreditAttribution: andrefy commentedhey jackbravo
I tested this module from a fresh Drupal installation. I think should be wort to add the date module as a requirement on the info file
I would expected that data_popup module will bring the date dependency but it doesn't, it brings date_api, and adding date as dependency make sense the widget is mean to be used on a field and also is under the readme.txt file.
Thanks the contribution, it is pretty useful, I would have used it on several previous projects.
Regards
Comment #19
jackbravo CreditAttribution: jackbravo commentedI added the dependency as you suggested. You are right, date_popup doesn't depend on date because it can be used as a stand alone widget only for views and not fields.
Thanks for testing @andrefy =)!
Comment #20
balis_m CreditAttribution: balis_m commentedIt' s really interesting module. Make date selections more users friendly.
I tested on the latest Drupal version and it works good.
I agree with @andrefy that the date module must be required and not the date_popup.
Comment #21
andrefy CreditAttribution: andrefy commentedMaybe the date_popup still needs to required since this module is build on the top of date_popup.
This module is working fine for me for Field Types:
- Date (Unix timestamp)
- Date (ISO format)
But I am having issues with the Field Type:
- Date
Steps to reproduce issue
- On a content type create a field type date and assign the Multiselect calendar
- Add a new content from that content type
After follow that steps I get the following error.
Issue Image
It seems that something is missing, the Pop-up calendar seems to work fine with the Date field type. Based on the error seems that is is always using a unix time stamp format instead of a date format.
Regards
Comment #22
andrefy CreditAttribution: andrefy commentedComment #23
jackbravo CreditAttribution: jackbravo commentedThanks for the review @andrefy!
I finally got some time to work on this and I fixed the problem. Ineed I had only tested using a unix timestamp date format. But now it works with all 3: date, date iso, unix timestamp. Only 2 functions needed changing =).
Comment #24
ajalan065 CreditAttribution: ajalan065 commentedDrupal already has a module named Datepicker which performs the task as is mentioned in your module.. Then what extra benefit do your module gives???
Comment #25
jackbravo CreditAttribution: jackbravo commentedHi ajalan065. The datepicker module and the date_popup module allow you to use a date javascript widget to select a date, but they only allow you to select one date per widget. So if you set up a field that allows multiple values, you will use several widgets to select multiple dates. Using this module you use only one widget.
In the [module page](https://www.drupal.org/sandbox/jackbravo/2417391) it says:
> It allows you to select multiple dates (with no time) in just one field, instead of having to add multiple popup widgets.
But maybe that is not clear enough. I added a screenshot to also show what the module does. Tell me if that helps. And if you have any suggestion on how to improve the description I'm also all ears =).
Comment #26
ajalan065 CreditAttribution: ajalan065 commentedThis module works fine by me
Comment #27
DevElCuy CreditAttribution: DevElCuy at Dilygent commentedGreat work @jackbravo!
Comment #28
cweagansThanks for your contribution!
I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.
Here are some recommended readings to help with excellent maintainership:
You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!
Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.
Thanks to the dedicated reviewer(s) as well.
Comment #29
jackbravo CreditAttribution: jackbravo commentedYay! Thanks a lot to all the reviewers!!!
https://www.drupal.org/project/date_multiselect =D.