Skip to content
This repository was archived by the owner on May 28, 2019. It is now read-only.

fix(ui-date): update to work with 1.3.x #99

Merged
merged 4 commits into from
Nov 10, 2014

Conversation

alexanderchan
Copy link
Contributor

This incorporates #94 as well the fixes from #96 as suggested by @giannisp and currently removes the blur as suggested in #95. I believe the switch to use modelValue is necessary if we are comparing it against a date as the angular docs say that viewValue should be a string.

This at least passes all tests using 1.3.0 and 1.2.5 however I am still concerned that we are removing the blur to workaround what might be related to issue angular/angular.js#8762.

Still I am submitting this to hopefully get closer to a full solution and this full pull request will at least pass all the tests. If someone has some suggestions on a good test case for when we need the blur that would be great and we can enhance this further.

Verified

This commit was signed with the committer’s verified signature.
ggerganov Georgi Gerganov
@alexanderchan
Copy link
Contributor Author

I think the blur should remain for validation trigger compatibility and to make sure that the $touched value is set.

I see for us where the breaking change happened for the blur that runs the extra $apply,

and essentially we need a similar fix as what was done in.

angular/angular.js@719c747#diff-9e383db252a4fb4b0fe122fe63727ff3R73

and then we can bring back the ui-date blur.

@alexanderchan
Copy link
Contributor Author

@wbeange I've created angular/angular.js#9808 and it resolves the blur issue so hopefully it will be accepted.

@ghost
Copy link

ghost commented Nov 4, 2014

Hey Alexander, I used your code to fix the issue of the ui-date-format error in angular 1.3.0.
The code in the ui-date-format directive seemed to have fixed the problem there, but in the ui-date directive you changed line 59 from
var date = controller.$viewValue;
to
var date = controller.$modelValue;
This is causing the value to be set to the original string date instead of the date object ui-date-format converted it to, which is causing it to throw an error every time.

@alexanderchan
Copy link
Contributor Author

Hi @miked1090 can you help by creating a jsbin/plunkr or test case of where the error occurs and I can try to see what is happening? Unfortunately I only have a basic example of how I'm using it where ng-model="someDateObject" and the tests that were written.

@ghost
Copy link

ghost commented Nov 4, 2014

Sure thing!
http://jsfiddle.net/bwq82bvs/16/

@alexanderchan
Copy link
Contributor Author

@miked1090 thanks for the example, that makes it more clear and perhaps there's something more to be done to make the render work more like the angular-ui bootstrap parsing and formatting.

ui-date:

      // Update the date picker when the model changes
      controller.$render = function () {
        var date = controller.$viewValue;
        if ( angular.isDefined(date) && date !== null && !angular.isDate(date) ) {
          throw new Error('ng-Model value must be a Date object - currently it is a ' + typeof date + ' - use ui-date-format to convert it from a string');
        }
        element.datepicker("setDate", date);
      };

ui.bootstrap.datepicker, here the viewvalue is being parsed directly:

  ngModel.$render = function() {
    var date = ngModel.$viewValue ? dateFilter(ngModel.$viewValue, dateFormat) : '';
    element.val(date);
    scope.date = parseDate( ngModel.$modelValue );
  };

also from the controller in ui.bootstrap.datepicker, here the modelValue is being moved directly into a date object (but will accept string and numeric values as we can see in the error message):

this.render = function() {
    if ( ngModelCtrl.$modelValue ) {
      var date = new Date( ngModelCtrl.$modelValue ),
      isValid = !isNaN(date);

      if ( isValid ) {
        this.activeDate = date;
      } else {
            $log.error('Datepicker directive: "ng-model" value must be a Date object, a number of milliseconds since 01.01.1970 or a string representing an RFC2822 or ISO 8601 date.');
      }
  ngModelCtrl.$setValidity('date', isValid);
}
this.refreshView();
};

I don't know the original reason for why there are two directives used on ui-date but I think the solution probably lies in adapting some of what is in the ui-bootstrap version of parsing of the modelValue using the configuration passed into the ui-date-format. I probably won't have time to dig into this anytime further soon so hopefully someone can build on this.

@dagroe dagroe mentioned this pull request Nov 6, 2014

Verified

This commit was signed with the committer’s verified signature.
ggerganov Georgi Gerganov
This refactors the date parsing to allow it to be used in the ui-date
directive.

Breaking Change the blur event isn't fired.  This can be added back when angular/angular.js#9808 is merged

Verified

This commit was signed with the committer’s verified signature.
ggerganov Georgi Gerganov
@alexanderchan
Copy link
Contributor Author

Hi @miked1090, I thought about this some more and I've refactored this a bit to be able to parse the date format in multiple places.

I'm not sure if people use the ui-date-format directive without the ui-date directive but I've left it as a directive in case it is.

Verified

This commit was signed with the committer’s verified signature.
ggerganov Georgi Gerganov
@alexanderchan alexanderchan self-assigned this Nov 9, 2014
@alexanderchan alexanderchan changed the title Fixes to work with 1.3.0 fix(ui-date): update to work with 1.3.x Nov 10, 2014
@alexanderchan alexanderchan added this to the 0.0.5 milestone Nov 10, 2014
alexanderchan pushed a commit that referenced this pull request Nov 10, 2014

Verified

This commit was signed with the committer’s verified signature.
ggerganov Georgi Gerganov
fix(ui-date): update to work with 1.3.x

fixes #95
fixes #96
@alexanderchan alexanderchan merged commit fb52c44 into angular-ui:master Nov 10, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant