Replace jscalendar with bootstrap-datetimepicker library #943

Closed
wants to merge 8 commits into
from

Projects

None yet

6 participants

@syncguru
Contributor
syncguru commented Nov 7, 2016 edited

bootstrap-datetimepicker: https://github.com/eonasdan/bootstrap-datetimepicker

  • Enable datetimepciker on all pages by default
  • Relocate inline JS code into standalone files
  • Remove CSP exceptions for jscalendar
  • Multi-language support

Fixes #20040

Date Picker
screen shot 2016-11-06 at 7 20 54 pm

Time Picker
screen shot 2016-11-06 at 7 23 22 pm

screen shot 2016-11-06 at 7 21 35 pm

screen shot 2016-11-06 at 7 22 07 pm

Bulk Edit
screen shot 2016-11-06 at 7 22 50 pm

French
screen shot 2016-11-06 at 7 32 36 pm

@syncguru syncguru Replace jscalendar with bootstrap-datetimepicker library
+ Enable datetimepciker on all pages by default
+ Relocate inline JS code into standalone files
+ Remove CSP exceptions for jscalendar
+ Multi-language support

Fixes #20040
2ca5023
@dregad

The new css/js files are not small... Is it necessary to include them on every page ?

config_defaults_inc.php
*/
-$g_calendar_js_date_format = '\%Y-\%m-\%d \%H:\%M';
+$g_calendar_js_date_format = 'Y-M-D H:m';
@dregad
dregad Nov 7, 2016 Member

For consistency with prior version (month and day with leading 0), the new format should be Y-MM-DD.

@dregad
Member
dregad commented Nov 7, 2016

Note: this was just a quick review, I have not yet tested the new functionality.

@atrol
Member
atrol commented Nov 7, 2016

I didn't have a deeper look, but what's wrong using the jquery-ui date-time picker used in PR #628/ ?
At first sight I see just that we introduce one more 3rd party dependency and that we have to deliver even more .js files to the client.

@syncguru
Contributor
syncguru commented Nov 7, 2016

@atrol Do you know why we include jquery-ui?
This PR removes jscalendar library and replace it with new one so net change is zero.

I opened issue #21881 to see if we can get rid of jquery-ui

@atrol
Member
atrol commented Nov 7, 2016

jquery and jquery-ui have been plugins before 1.3.x
https://github.com/mantisbt-plugins/jquery
https://github.com/mantisbt-plugins/jQuery-UI

They have been bundled with MantisBT core to make life of plugin authors a bit easier.

At least this one uses jQuery-UI
https://github.com/mantisbt-plugins/Inline-column-configuration

@vboctor
Member
vboctor commented Nov 7, 2016

@syncguru as we replace the date time control, we should do one of two things:

  1. Deprecate js calendar configs and add new ones.
  2. Do the internal processing to convert existing config options format to the new one, so existing configurations would work.

Please update library/README.md with new libraries used and their version details.

I will do some testing tonight and provide more feedback.

@atrol I haven't researched the components being compared here, but I would rather use a bootstrap based component over a jquery-ui based one. The component used here has > 4,800 stars. That is pretty popular. Will look more.

@syncguru Did you test on small screens?

But generally, I'm happy with finally getting rid of the outdated jscalendar and the inline scripts associated with it.

@syncguru
Contributor

@dregad

  • Fixed the date format
  • JS files download are a onetime cost (in addition to cdn support). The logic that existed before to detect the use of the calendar and then include the js/css files is complex, error prone and not plugin friendly. It is hard to imaging a bug tracker without a date/time picker field.

@vboctor

  • Deprecated js calendar config
  • There is no new library added under library directory. jscalander did not have an entry in the readme file for me to remove.
  • Yes, the control works on mobile. For MantisBt, it is a shifted to the right as a by product of the table format. This will auto resolve as soon as we optimize tables/forms for mobile.

@atrol
I strongly think we should remove jquery-ui ... happy to fix broken plugins. It should be very easy fix.

@atrol
Member
atrol commented Nov 11, 2016

I strongly think we should remove jquery-ui

I don't have a problem with it as I don't use any plugin that uses jquery-ui, but would like to get also some feedback from others, e.g. @rombert who is the author of the Inline-column-configuration plugin.

@atrol
Member
atrol commented Nov 13, 2016

manage_proj_ver_edit_page.php does not work for me.
bug_update_page.php (Due Date) works when clicking in the text field, but not when clicking on the calendar icon.

@dregad

Replacement of jscalendar should allow removal of CSP bypass for bug report page, see https://github.com/mantisbt/mantisbt/blob/release-2.0.0-rc.1/core/http_api.php#L236, please confirm and implement it as part of the PR if no longer needed.

@badfiles
Contributor

I think inputs with datetimepicker should not be auto-completed (autocomplete="off"), in case if js code does not already turn off this feature.

@dregad
Member
dregad commented Nov 14, 2016

I strongly think we should remove jquery-ui ... happy to fix broken plugins. It should be very easy fix.

I don't have a problem with it as I don't use any plugin that uses jquery-ui, but would like to get also some feedback from others, e.g. @rombert who is the author of the Inline-column-configuration plugin.

As far as plugins registered in mantisbt-plugins org, I found several that refer to or use jQuery-UI in one way or another. Mentioning their respective authors to bring their attention to this discussion, and get their opinion:

Most of these plugins have not been updated recently, and none have yet been revised by their respective authors to work with MantisBT 2.x.

Since we don't use jQuery-UI ourselves in the core ATM (and it probably does not make sense to do so in the future since we went down the Bootstrap route), I don't have a strong objection against removing it; it would be quite straightforward to update the jQuery-UI plugin, to enable 3rd-party plugins to use that functionality if they need it.

@rombert
Member
rombert commented Nov 14, 2016

Hm, managed to miss the mention. Anyway, I don't see a problem with removing jQuery-UI and having administrators pull in the jQuery-UI plug-in themselves if needed.

- # The JS Calendar control does unsafe eval, remove once we upgrade the control (see #20040)
- if( 'bug_update_page.php' == basename( $_SERVER['SCRIPT_NAME'] ) ) {
- http_csp_add( 'script-src', "'unsafe-eval'" );
}
@syncguru
syncguru Nov 20, 2016 Contributor

@dregad fyi: csp exception no longer needed.

@dregad
dregad Nov 21, 2016 Member

Cheers

@syncguru
Contributor

@atrol

  • Good catch, fixed.
  • The calendar icon is not clickable due to some css complexities. This should not block this PR though, will try to fix in a separate PR.
@dregad

This should not block this PR though

I disagree. The button was operational before, and changes in this PR broke it. Making it work as before must be done here so we don't introduce a regression.

@vboctor

@syncguru I did a quick test + review and here is my feedback:

  • I agree with @dregad that the calendar icon should work.
  • The new configuration options are not documented in the manual.
  • Should we use new configs or try to re-use the standard MantisBT configuration options for date formats, i.e. normal_date_format, short_date_format or complete_date_format? I would rather re-use one of these rather than creating new ones.
  • When using the calendar control to set the date, it adds extra spaces/tabs at the end of the field. Can we remove these?
  • Do we control the default time? e.g. can the time be 00:00 by default rather than current time? Not sure about the correct behavior here. Not sure what was the default behavior for jscalendar.
core/lang_api.php
+
+/**
+ * Maps current lang string to moment.js locale https://github.com/moment/moment/tree/develop/locale
+ * @return string
@vboctor
vboctor Nov 22, 2016 Member

Document what the return string will contain.

core/lang_api.php
+ * Maps current lang string to moment.js locale https://github.com/moment/moment/tree/develop/locale
+ * @return string
+ */
+
@vboctor
vboctor Nov 22, 2016 Member

Remove blank line between phpdoc and function.

core/layout_api.php
@@ -307,11 +313,21 @@ function layout_head_javascript() {
* @return null
*/
function layout_body_javascript() {
- # bootstrap
+
@vboctor
vboctor Nov 22, 2016 Member

Remove extra blank line.

@syncguru
Contributor

@dregad, @vboctor - Addressed your comments.

core/obsolete.php
@@ -203,4 +203,6 @@
# changes in 2.0.0dev
config_obsolete( 'icon_path' );
config_obsolete( 'bug_print_page_fields' );
+config_obsolete( 'calendar_js_date_format' );
@atrol
atrol Dec 4, 2016 Member

should be
config_obsolete( 'calendar_js_date_format', 'datetime_picker_format' );

core/obsolete.php
@@ -203,4 +203,6 @@
# changes in 2.0.0dev
config_obsolete( 'icon_path' );
config_obsolete( 'bug_print_page_fields' );
+config_obsolete( 'calendar_js_date_format' );
+config_obsolete( 'calendar_date_format' );
@atrol
atrol Dec 4, 2016 Member

Should be
config_obsolete( 'calendar_date_format', 'datetime_picker_format' );

@syncguru syncguru Misc tweaks based on code review comments
1- Update obsolete statements
2- Reduce datepicker box width to avoid icon wrapping

Fixes #20040
44102be
@syncguru
Contributor
syncguru commented Dec 4, 2016

@atrol - Done
@dregad, @vboctor - can we get this in for today's release?

@syncguru syncguru Merge branch 'master' of https://github.com/syncguru/mantisbt into 20…
…040-datetime-picker

# Conflicts:
#	core/obsolete.php
7dd133a
@vboctor
Member
vboctor commented Dec 19, 2016

@dregad Please have a look that your feedback is addressed and merge once ready.

@dregad dregad added a commit that referenced this pull request Dec 20, 2016
@dregad dregad Replace jscalendar with bootstrap datetimepicker
Pull request #943
0253f46
@dregad
dregad approved these changes Dec 20, 2016 View changes
@dregad
Member
dregad commented Dec 20, 2016

Merged in 0253f46

@dregad dregad closed this Dec 20, 2016
@dregad dregad added the merged label Dec 20, 2016
@syncguru syncguru deleted the syncguru:20040-datetime-picker branch Jan 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment