Fixes: U4-9273 Add date picker to query builder #1648
Conversation
There was a problem hiding this comment.
Code seems OK, just a couple questions I have if you don't mind answering @madsrasmussen for clarity please.
| pickDate: true, | ||
| pickTime: true, | ||
| useSeconds: true, | ||
| format: "YYYY-MM-DD HH:mm:ss", |
There was a problem hiding this comment.
@madsrasmussen with this being a date and a time picker we may need to add a new property for timezones.
There was a problem hiding this comment.
Also this example in the docs does it show all possible options/config values that can be used?
There was a problem hiding this comment.
I don't think we should just modify the dates inside the directive? I can see it uses moments for something but haven't really spend time on it.
The docs doesn't show all the config options but I have added a link to the original documentation.
|
|
||
| function onInit() { | ||
| // load css file for the date picker | ||
| assetsService.loadCss('lib/datetimepicker/bootstrap-datetimepicker.min.css').then(function () { |
There was a problem hiding this comment.
Is there any need for the then() as we are not doing anything with the promise once the CSS loaded/resolved
There was a problem hiding this comment.
It is now removed :)
| if (scope.onChange && event.date && event.date.isValid()) { | ||
| scope.$apply(function(){ | ||
| // Update ngModel | ||
| scope.ngModel = event.date.format(scope.options.format); |
There was a problem hiding this comment.
Is the format used for display purposes or is the format used when being sent back to the server endpoints, as to make life easier with datetimes, formats & timezone hell, I think it may need to be consistently sent to the server in a format that is agreed upon, as if you were to change it clientside the server side may interpret the date as the totally wrong month.
There was a problem hiding this comment.
I have changed this code a bit because of some timing issues. You now have to use the raw date from one of the callbacks and can do whatever is needed with that. I hope it makes sense 👍
| element | ||
| .datetimepicker(angular.extend({ useCurrent: true }, scope.options)) | ||
| .on("dp.change", onChange) | ||
| .on("dp.error", onError); |
There was a problem hiding this comment.
Does the underlying library that is being used have any other useful events that are being fired that might be useful for anyone else using this component & worth adding?
There was a problem hiding this comment.
I have added the rest of the events from the documentation also: http://eonasdan.github.io/bootstrap-datetimepicker/Events/
|
|
||
|
|
||
| function datePickerChange(event) { | ||
| templateQueryResource.postTemplateQuery(vm.query) |
There was a problem hiding this comment.
Correct me if I am wrong @madsrasmussen but the main directive model is filter.constraintValue and I was expecting this function to update vm.query to include the chosen date to post to the server for the query to be made.
There was a problem hiding this comment.
The value was updated on ngModel but there were some timing issues which f***** it up. I have changed the code a bit so you now have to use the date which comes from the callback. If I understand you correctly, the code is now what you expected to see and not so much angular magic :)
| @@ -0,0 +1,143 @@ | |||
| /** | |||
| @ngdoc directive | |||
There was a problem hiding this comment.
Perhaps with the doc comment, indicate what version it will be for. In case package devs try to use it in an older version of Umbraco.
There was a problem hiding this comment.
It is now added to the docs 👍
|
@madsrasmussen maybe there are some stuff in my previously PR here you can re-use? #984 |
|
@bjarnef As far as I understand your PR, it is for exposing all the different config options in a UI. What I am doing here is wrapping the load, init and events of the date picker in a directive so the code is a bit cleaner and easier to reuse. But thanks for your input 👍 I hope there will be time to go through all the waiting PR's soon and then we make everything work together. |
|
@madsrasmussen yes, I just noticed the config where it set in my PR I changed these config option based on the |
| } | ||
|
|
||
| function onUpdate(event) { | ||
| if (scope.onShow) { |
There was a problem hiding this comment.
Is this correct @madsrasmussen I would have thought it should check if we have a function set in scope.onUpdate which we then run it
| } | ||
|
|
||
| function onHide(event) { | ||
| if (scope.onChange) { |
There was a problem hiding this comment.
Is this correct @madsrasmussen I would have thought it should check if we have a function set in scope.onHide which we then run it
| @@ -1,14 +1,20 @@ | |||
| (function () { | |||
| "use strict"; | |||
|
|
|||
| function QueryBuilderOverlayController($scope, templateQueryResource) { | |||
| function QueryBuilderOverlayController($scope, $element, templateQueryResource, assetsService, angularHelper) { | |||
There was a problem hiding this comment.
The three extra injected dependencies don't seem to be used in this file so best to be neat & tidy and remove them please @madsrasmussen
|
@warrenbuckley good catches. They are fixed now 👍 |
|
Thanks for the speedy changes - currently testing creating a property editor using this directive to ensure things are wired up. |
|
OK after reading the wrong/old docs from an earlier commit & wondering why I could not get ng-model to work for the directive in my prop editor test. Only to realise doc was out of date I was reading & that Mads has given an example using the change event fired by the underling date picker JS library. All tested & code sanity checked. All happy & merging in. |
No description provided.