Replace jscalendar with bootstrap-datetimepicker library #943

syncguru · 2016-11-07T03:37:15Z

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

Time Picker

Bulk Edit

French

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

dregad

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

dregad · 2016-11-07T11:10:22Z

config_defaults_inc.php

 */
-$g_calendar_js_date_format = '\%Y-\%m-\%d \%H:\%M';
+$g_calendar_js_date_format = 'Y-M-D H:m';


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

dregad · 2016-11-07T11:28:44Z

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

atrol · 2016-11-07T13:46:33Z

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 · 2016-11-07T15:27:35Z

@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 · 2016-11-07T15:41:19Z

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 · 2016-11-07T15:58:06Z

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

Deprecate js calendar configs and add new ones.
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.

Fixes #20040

syncguru · 2016-11-11T04:10:43Z

@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 · 2016-11-11T23:23:31Z

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 · 2016-11-13T11:56:32Z

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 · 2016-11-14T11:58:26Z

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 · 2016-11-14T12:08:44Z

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:

jQuery-UI (duh)
customer-management by @rombert
Inline-column-configuration by @rombert (as previously mentioned)
jsedit by @Kirill
agileMantis by gadiv (@jako87, @jazsch, @jssw) but in this case the plugin bundles jQuery-UI as part of its source

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 · 2016-11-14T12:21:01Z

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.

Fixes #20040

syncguru · 2016-11-20T17:06:25Z

core/http_api.php


-		# 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'" );
 		}


@dregad fyi: csp exception no longer needed.

syncguru · 2016-11-20T17:07:31Z

@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.

vboctor · 2016-11-22T04:36:35Z

core/lang_api.php

+
+/**
+ * Maps current lang string to moment.js locale https://github.com/moment/moment/tree/develop/locale
+ * @return string


Document what the return string will contain.

vboctor · 2016-11-22T04:36:53Z

core/lang_api.php

+ * Maps current lang string to moment.js locale https://github.com/moment/moment/tree/develop/locale
+ * @return string
+ */
+


Remove blank line between phpdoc and function.

vboctor · 2016-11-22T04:37:27Z

core/layout_api.php

@@ -307,11 +313,21 @@ function layout_head_javascript() {
 * @return null
 */
 function layout_body_javascript() {
-	# bootstrap
+


Remove extra blank line.

Fixes #20040

…antisbt into 20040-datetime-picker

Fixes #20040

syncguru · 2016-11-24T00:50:41Z

@dregad, @vboctor - Addressed your comments.

atrol · 2016-12-04T13:33:24Z

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' );


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

atrol · 2016-12-04T13:33:56Z

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' );


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

1- Update obsolete statements 2- Reduce datepicker box width to avoid icon wrapping Fixes #20040

syncguru · 2016-12-04T20:21:50Z

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

…040-datetime-picker # Conflicts: # core/obsolete.php

vboctor · 2016-12-19T17:21:20Z

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

Pull request #943

dregad · 2016-12-20T17:50:37Z

Merged in 0253f46

Replace jscalendar with bootstrap-datetimepicker library

2ca5023

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

dregad reviewed Nov 7, 2016

View reviewed changes

Obsolete jscalendar global config & use compatible date format

ed979aa

Fixes #20040

dregad requested changes Nov 14, 2016

View reviewed changes

Enable datetime picker for project version edit box

ed68855

Fixes #20040

syncguru commented Nov 20, 2016

View reviewed changes

dregad requested changes Nov 21, 2016

View reviewed changes

vboctor requested changes Nov 22, 2016

View reviewed changes

Rafik Robeal added 3 commits November 23, 2016 13:40

Show datetime picker when clicking calendar icon

9a26dec

Fixes #20040

Merge branch '20040-datetime-picker' of https://github.com/syncguru/m…

e51f262

…antisbt into 20040-datetime-picker

Misc tweaks based on code review feedbacks

f92093d

Fixes #20040

atrol reviewed Dec 4, 2016

View reviewed changes

Misc tweaks based on code review comments

44102be

1- Update obsolete statements 2- Reduce datepicker box width to avoid icon wrapping Fixes #20040

Merge branch 'master' of https://github.com/syncguru/mantisbt into 20…

7dd133a

…040-datetime-picker # Conflicts: # core/obsolete.php

vboctor approved these changes Dec 12, 2016

View reviewed changes

dregad added a commit that referenced this pull request Dec 20, 2016

Replace jscalendar with bootstrap datetimepicker

0253f46

Pull request #943

dregad approved these changes Dec 20, 2016

View reviewed changes

dregad closed this Dec 20, 2016

dregad added the merged label Dec 20, 2016

badfiles mentioned this pull request Dec 21, 2016

Replace jscalendar with jquery-ui datetimepicker #628

Closed

syncguru deleted the 20040-datetime-picker branch January 24, 2017 02:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace jscalendar with bootstrap-datetimepicker library #943

Replace jscalendar with bootstrap-datetimepicker library #943

syncguru commented Nov 7, 2016 •

edited

Loading

dregad left a comment

dregad Nov 7, 2016

dregad commented Nov 7, 2016

atrol commented Nov 7, 2016

syncguru commented Nov 7, 2016

atrol commented Nov 7, 2016

vboctor commented Nov 7, 2016

syncguru commented Nov 11, 2016

atrol commented Nov 11, 2016

atrol commented Nov 13, 2016

dregad left a comment

badfiles commented Nov 14, 2016

dregad commented Nov 14, 2016

rombert commented Nov 14, 2016

syncguru Nov 20, 2016

dregad Nov 21, 2016

syncguru commented Nov 20, 2016

dregad left a comment

vboctor left a comment

vboctor Nov 22, 2016

vboctor Nov 22, 2016

vboctor Nov 22, 2016

syncguru commented Nov 24, 2016

atrol Dec 4, 2016

atrol Dec 4, 2016

syncguru commented Dec 4, 2016

vboctor commented Dec 19, 2016

dregad commented Dec 20, 2016

Replace jscalendar with bootstrap-datetimepicker library #943

Replace jscalendar with bootstrap-datetimepicker library #943

Conversation

syncguru commented Nov 7, 2016 • edited Loading

dregad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dregad commented Nov 7, 2016

atrol commented Nov 7, 2016

syncguru commented Nov 7, 2016

atrol commented Nov 7, 2016

vboctor commented Nov 7, 2016

syncguru commented Nov 11, 2016

atrol commented Nov 11, 2016

atrol commented Nov 13, 2016

dregad left a comment

Choose a reason for hiding this comment

badfiles commented Nov 14, 2016

dregad commented Nov 14, 2016

rombert commented Nov 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syncguru commented Nov 20, 2016

dregad left a comment

Choose a reason for hiding this comment

vboctor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syncguru commented Nov 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syncguru commented Dec 4, 2016

vboctor commented Dec 19, 2016

dregad commented Dec 20, 2016

syncguru commented Nov 7, 2016 •

edited

Loading