feat(ng-model): support input type=date | datetime and all other date/time variants #747

chalin · 2014-03-17T23:43:46Z

Add support for input elements of type date|datetime|datetime-local|month|time|week.
Remove non-UTF-8 characters from some other API comments.

chalin · 2014-03-18T01:00:37Z

As is mentioned in the API documentation for InputDateLikeDirective, date related inputs are not fully supported yet nor does the support always conform to the HTML standard. This required careful design in the behavior of InputDateLikeDirective (see the API doc for details). It also made writing tests a challenge. Tests pass under Dartium (stable and dev), but not under dart2js. Now that I finally have Chrome/dart2js tests running locally, I might need some hints as to how to best approach debugging dart2js test failures. Marking this [WIP] until the dart2js tests pass.

vicb · 2014-03-18T09:57:20Z

lib/directive/ng_model.dart

+ */
+@NgDirective(selector: '[ng-value-as]')
+class NgDateLikeValueAsDirective {
+  static const String NG_VALUE_AS = 'ng-value-as',


what about multiple lines for readability ?

chalin · 2014-03-19T03:34:12Z

Updates done. Tests failing under JS have been disabled. I will create a small test case and file a bug tomorrow.

chalin · 2014-03-19T12:53:46Z

Removed [WIP] status; I think that this version should be good to merge. If not let me know what else is needed.

chalin · 2014-03-19T16:43:02Z

FYI: Issue 17625: Reading InputElement.valueAsDate results in NullError (Cannot call "getTime" on null), but should just be null value. Once this is fixed I will be able to re-enable the disabled tests.

vicb · 2014-03-20T11:16:06Z

lib/directive/ng_model.dart

+@NgDirective(selector: '[ng-value-as]')
+class NgDateLikeValueAsDirective {
+  static const NG_VALUE_AS = 'ng-value-as',
+    BEST = 'best', 


indentation

Not sure what your preference is: to bring the NG_VALUE_AS down a line and match indentation for the rest or to align all of BEST etc with NG_V*?

Well my preference would be to repeat static const but by CS your are only required to 4+ ws a on a continuation line. Change for whatever your prefer.

chalin · 2014-03-20T11:34:46Z

Btw, we should use an (pre-)checkin autoformatter. I have had that on other projects. It saves everyone some time.

vicb · 2014-03-20T11:39:48Z

Yep I agree. I'm not sure if we should really autoformat but at least send the commit author an email with a patch.

mhevery · 2014-03-20T18:06:36Z

test/directive/ng_model_spec.dart

+
+      inputType = 'date';
+      describe('type=$inputType', () {
+        _itWorksM2E(inputType, null, stripTime, setValAsDateTime);


Our style is to keep it methods in the describe. Currently this style is confusing since it is not clear what exactly is being teste. From within the it you are free to call helper methods.

mhevery · 2014-04-01T03:15:02Z

test/directive/ng_model_datelike_spec.dart

+// Test driver:
+
+void main() {
+  new NgModelDateLikeTester().run();


There is no need for this extra class. It is not how the rest of our tests our set up. Please follow our convertions.

main() { describe('ng-model for data-like', () { describe('subsection', () { it('should do semething', () { ...; }); }); }); }

Ok. I figured it was worth a try ... of illustrating a slightly different approach that can benefit from use of the Dart Editor:

outline mode (see the example below)

editor view fold feature (the minus inside a circle in the left margin)

Some of the main test functions are monolithic: e.g. main() is 1500 lines in ng_model_spec.dart, and the Dart Editor can't help navigating it (other than just scrolling).

Using a class allows the test fixture to be set as fields and each second-level describe() as a method, which makes it easier to visualize grouping: e.g.,

mhevery · 2014-04-01T03:53:24Z

Hi @chalin this is getting very close. I left few more comments, biggest one is to use null value pattern rather then check for nulls and remove the extra Test class you have. Please incorporate the comments, and I will merge it in.

chalin · 2014-04-03T16:50:34Z

@mhevery : done. Also, I removed the Directive suffix on the class names, since those are on the way out.

mhevery · 2014-04-04T04:41:50Z

I tried to merge this in (with some changes) but it breaks the travis.
Travis: https://travis-ci.org/angular/angular.dart/jobs/22241686
Code: https://github.com/angular/angular.dart/tree/broken-pr-747

The issue is that dart2js transformer can't seem to handle the annotation. @blois can you look into it?

chalin · 2014-04-04T11:31:22Z

Help would be appreciated. I tried to figure out what was going on but was unable to (and now I need to shift to doing some academic work). I'll chime in later today.

vicb · 2014-04-04T11:33:36Z

You should first try to rebase on latest master.

chalin · 2014-04-04T12:23:11Z

Done.

chalin · 2014-04-04T12:50:40Z

@vicb : rebasing didn't help, I get the same error

Building angular_dart_example..............................
Build error:
Transform ExpressionGenerator on angular_dart_example|web/todo.dart threw error: Class 'SimpleIdentifier' has no instance getter 'stringValue'.

chalin · 2014-04-04T13:13:11Z

Maybe #856 has something to do with it?

blois · 2014-04-04T17:13:04Z

The build issue is this @NgAttr(DIRECTIVE_NAME)- right now source_metadata_extractor.dart does not follow property references in the annotations.

It's a little complex right now because this code is used from both the command-line expression_extractor.dart and the Angular transformers- the transformers are using a resolved AST which would allow us to evaluate the expression, but the command-line utilities are using an unresolved AST.

Proper fix would be to update expression_extractor to use a resolved AST so we can evaluate the expression, but this is a fairly involved change- ideally refactor the command-line tool to just wrap the transformer.

The easiest fix (and what I'd recommend going with for now) is to use a string literal in the annotation rather than the const reference.

Separately I'm investigating why the test-expression-extractor.sh script is not failing on this- there appear to some issues there where it's not analyzing the entire application (the file that gets generated by the test is missing a LOT of Angular expressions).

chalin · 2014-04-04T19:06:13Z

Good bug hunting! @mhevery shall I make the tweak or is this something you will handle when merging?

chalin · 2014-04-04T20:34:38Z

I made the change in @NgAttr(), but the Travis build is still failing, though now with different errors:

build/web/animation.dart.js is too large expecting 205753 was 207946.
build/web/bouncing_balls.dart.js is too large expecting 200130 was 202260.
build/web/hello_world.dart.js is too large expecting 197883 was 199863.
build/web/todo.dart.js is too large expecting 200783 was 203051.

chalin · 2014-04-04T22:57:00Z

Yay! Glad to get this one wrapped up.

chalin added cla: yes and removed cla: no labels Mar 17, 2014

chalin closed this Mar 18, 2014

chalin reopened this Mar 18, 2014

chalin changed the title ~~feat(ng-model): support input type=date | datetime and all other date/time variants~~ [WIP] feat(ng-model): support input type=date | datetime and all other date/time variants Mar 18, 2014

chalin mentioned this pull request Mar 18, 2014

ng-model does handle <input type="date"> #407

Closed

vicb reviewed Mar 18, 2014
View reviewed changes

chalin changed the title ~~[WIP] feat(ng-model): support input type=date | datetime and all other date/time variants~~ feat(ng-model): support input type=date | datetime and all other date/time variants Mar 19, 2014

vicb reviewed Mar 20, 2014
View reviewed changes

mhevery reviewed Mar 20, 2014
View reviewed changes

mhevery reviewed Apr 1, 2014
View reviewed changes

chalin mentioned this pull request Apr 2, 2014

refactor(InputCheckboxDirective): fix API doc and refactor ng-true/false-value #842

Closed

blois mentioned this pull request Apr 4, 2014

Fixing expression extractor & test #860

Closed

mhevery closed this in 90e0e07 Apr 4, 2014

chalin deleted the patch-0315-date-input-ng-model branch April 4, 2014 22:57

feat(ng-model): support input type=date | datetime and all other date/time variants #747

feat(ng-model): support input type=date | datetime and all other date/time variants #747

Uh oh!

Conversation

chalin commented Mar 17, 2014

Uh oh!

chalin commented Mar 18, 2014

Uh oh!

vicb Mar 18, 2014

Choose a reason for hiding this comment

Uh oh!

chalin commented Mar 19, 2014

Uh oh!

chalin commented Mar 19, 2014

Uh oh!

chalin commented Mar 19, 2014

Uh oh!

vicb Mar 20, 2014

Choose a reason for hiding this comment

Uh oh!

chalin Mar 20, 2014

Choose a reason for hiding this comment

Uh oh!

vicb Mar 20, 2014

Choose a reason for hiding this comment

Uh oh!

chalin commented Mar 20, 2014

Uh oh!

vicb commented Mar 20, 2014

Uh oh!

mhevery Mar 20, 2014

Choose a reason for hiding this comment

Uh oh!

mhevery Apr 1, 2014

Choose a reason for hiding this comment

Uh oh!

chalin Apr 1, 2014

Choose a reason for hiding this comment

Uh oh!

mhevery commented Apr 1, 2014

Uh oh!

chalin commented Apr 3, 2014

Uh oh!

mhevery commented Apr 4, 2014

Uh oh!

chalin commented Apr 4, 2014

Uh oh!

vicb commented Apr 4, 2014

Uh oh!

chalin commented Apr 4, 2014

Uh oh!

chalin commented Apr 4, 2014

Uh oh!

chalin commented Apr 4, 2014

Uh oh!

blois commented Apr 4, 2014

Uh oh!

chalin commented Apr 4, 2014

Uh oh!

chalin commented Apr 4, 2014

Uh oh!

chalin commented Apr 4, 2014

Uh oh!

Uh oh!