Change datetime to datetime-local #5664

jbagga · 2017-01-06T19:57:07Z

Addresses #4871

dougbu

Must also change the similar code in DefaultEditorTemplates. Nothing is shared for reasons…

dougbu · 2017-01-06T20:26:49Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/InputTagHelper.cs

@@ -57,7 +57,7 @@ public class InputTagHelper : TagHelper
            new Dictionary<string, string>(StringComparer.Ordinal)
            {
                { "date", "{0:yyyy-MM-dd}" },
-                { "datetime", "{0:yyyy-MM-ddTHH:mm:ss.fffK}" },
+                { "datetime", "{0:yyyy-MM-ddTHH:mm:ss.fff}" },


Don't change this. The bug is about the type attribute values MVC chooses when the <input> element has none. If users type in <input type="datetime" ... />, we still need to use the appropriate RFC 3339 format.

jbagga · 2017-01-10T01:56:39Z

@dougbu Updated

dougbu · 2017-01-10T16:34:32Z

test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/InputTagHelperTest.cs

@@ -248,7 +248,7 @@ public static TheoryData MultiAttributeCheckBoxData
        }

        [Theory]
-        [InlineData(null, "datetime")]
+        [InlineData(null, "datetime-local")]


Add a case for [InlineData("datetime", "datetime")] i.e. demonstrate user can choose <input type="datetime">. This might require a new test.

dougbu · 2017-01-10T16:36:27Z

test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/InputTagHelperTest.cs

-        [InlineData("NullableDateTime", Html5DateRenderingMode.Rfc3339, "{0:yyyy-MM-ddTHH:mm:ss.fffK}", "datetime")]
-        [InlineData("NullableDateTimeOffset", Html5DateRenderingMode.Rfc3339, "{0:yyyy-MM-ddTHH:mm:ss.fffK}", "datetime")]
+        [InlineData("NullableDateTime", Html5DateRenderingMode.Rfc3339, "{0:yyyy-MM-ddTHH:mm:ss.fff}", "datetime-local")]
+        [InlineData("NullableDateTimeOffset", Html5DateRenderingMode.Rfc3339, "{0:yyyy-MM-ddTHH:mm:ss.fff}", "datetime-local")]
        public async Task ProcessAsync_CallsGenerateTextBox_AddsExpectedAttributesForRfc3339(


Since the equivalent of [DataType("datetime-local")] is now the default, add checks of a new property with an associated [DataType("datetime")].

dougbu · 2017-01-10T16:49:08Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/DefaultEditorTemplates.cs

@@ -350,7 +350,7 @@ public static IHtmlContent EmailAddressInputTemplate(IHtmlHelper htmlHelper)
        public static IHtmlContent DateTimeInputTemplate(IHtmlHelper htmlHelper)
        {
            ApplyRfc3339DateFormattingIfNeeded(htmlHelper, "{0:yyyy-MM-ddTHH:mm:ss.fffK}");
-            return GenerateTextBox(htmlHelper, inputType: "datetime");
+            return GenerateTextBox(htmlHelper, inputType: "datetime-local");


This looks incorrect. Means users can't force generation of type="datetime" without htmlAttributes shenanigans. Also uses the wrong format for the chosen type value. If this is what we wanted, would probably remove DateTimeInputTemplate() and update this table.

In any case, won't need to change this class at all given my comments in InputTagHelper.

Not sure I understand this... I agree that perhaps if the user typed in <input type="datetime" ... blah blah /> that we should preserve that. But I think in all other cases we should start rendering type="datetime-local".

Ah, I thought I was toeing closer to the @Eilon line. See other thread on the subject.

dougbu · 2017-01-10T16:54:30Z

test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/DefaultEditorTemplatesTest.cs

@@ -769,7 +769,6 @@ public void Editor_FindsViewDataMember()
        // DateTime-local is not special-cased unless using Html5DateRenderingMode.Rfc3339.
        [Theory]
        [InlineData("date", "{0:d}", "2000-01-02")]
-        [InlineData("datetime", null, "2000-01-02T03:04:05.006+00:00")]
        [InlineData("datetime-local", null, "2000-01-02T03:04:05.006")]
        [InlineData("time", "{0:t}", "03:04:05.006")]
        public void Editor_FindsCorrectDateOrTimeTemplate(string dataTypeName, string editFormatString, string expected)


This test covers explicit [DataType(...)] use. Whether or not you agree [DataType("datetime")] should work differently than [DataType("datetime-local")], both should be tested. I.e. these cases should remain though expectations might change.

Same for Editor_AppliesRfc3339() and Editor_AppliesNonDefaultEditFormat().

dougbu · 2017-01-10T17:04:32Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/InputTagHelper.cs

@@ -32,7 +32,7 @@ public class InputTagHelper : TagHelper
                { "Url", "url" },
                { "EmailAddress", "email" },
                { "Date", "date" },
-                { "DateTime", "datetime" },
+                { "DateTime", "datetime-local" },


Thinking about this more, should narrow this change to exactly what @Eilon said:

change System.DateTime to render as <input type="datetime-local" ... />

So, remove this change and instead update TemplateRenderer.GetTypeNames(). Note InputTagHelper and DefaultEditorTemplates both use that method. Specifically, change TemplateRenderer lines 208-210 to return "datetime-local" for the DateTime and DateTimeOffset types. That way, [DataType(...)] should still work but the defaults will be valid.

Why wouldn't we change this here? I'm pretty sure we don't ever want to render type="datetime" because it is not spec-compliant, no?

There are 3 cases and I thought you wanted to change only (1) below. @jbagga's changes cover (1) and (2). If that's what you want, her source changes are fine and the main bit is confirming all 3 cases are covered for HTML and tag helpers.

Property has type DateTime or DateTimeOffset.

Property has some random type and an associated [DataType("datetime")].

Property has whatever type but user either entered <input type="datetime" ...> or included [type]: "datetime" in their htmlAttributes dictionary.

That is, @Eilon please confirm your intent. Obviously my leaning was toward changing only case (1) because (2) seems more like an explicit ask for type="datetime". But, I can see (2) is not that much more explicit.

@dougbu So the change in TemplateRenderer lines 208-210 to return "datetime-local" will cover (3)?

So the change in TemplateRenderer lines 208-210 to return "datetime-local" will cover (3)?

No, nothing you're changing will affect that case. The tag helper does not override an explicit type attribute and your changes as well as my suggestion stick to the realm of defaults.

jbagga · 2017-01-10T23:58:42Z

@dougbu I am not sure which PR comments (above) about the tests still apply. Most were related to (2), which we have decided to change behavior of. I apologize if the comments are still relevant. Please let me know and I will change them right away.

dougbu · 2017-01-11T00:42:00Z

@jbagga sorry, machine was partially frozen for a bit…

We should cover the (2) cases in our tests i.e. ensure [DataType(DataType.DateTime)] and [DataType("datetime-local")] now work as expected aka identically. That goes for both the HTML and tag helpers.

jbagga · 2017-01-11T20:37:00Z

@dougbu Updated

dougbu · 2017-01-11T23:57:48Z

test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/InputTagHelperTest.cs

+        [InlineData("Time", Html5DateRenderingMode.Rfc3339, "{0:HH:mm:ss.fff}", "time", "time")]
+        [InlineData("NullableDate", Html5DateRenderingMode.Rfc3339, "{0:yyyy-MM-dd}", "date", "date")]
+        [InlineData("NullableDateTime", Html5DateRenderingMode.Rfc3339, "{0:yyyy-MM-ddTHH:mm:ss.fff}", "datetime", "datetime-local")]
+        [InlineData("NullableDateTimeOffset", Html5DateRenderingMode.Rfc3339, "{0:yyyy-MM-ddTHH:mm:ss.fff}", "datetime", "datetime-local")]


Move new dataTypeName parameter and data before expectedFormat.

dougbu · 2017-01-11T23:58:41Z

test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/InputTagHelperTest.cs

@@ -967,7 +969,9 @@ public void Process_GeneratesFormattedOutput(string specifiedType, string expect
            {
                { "type", expectedType }


Change this line to use the new dataTypeName parameter. Don't make the changes a couple of lines below.

dougbu · 2017-01-12T00:02:45Z

test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/DefaultEditorTemplatesTest.cs

+        [InlineData("datetime", "datetime-local", null, "2000-01-02T03:04:05.060")]
+        [InlineData("datetime-local", "datetime-local", null, "2000-01-02T03:04:05.060")]
+        [InlineData("time", "time", "{0:t}", "03:04:05.060")]
+        public void Editor_AppliesRfc3339(string dataTypeName, string expectedType, string editFormatString, string expected)


Move expectedType last or right after editFormatString.

dougbu · 2017-01-12T00:05:48Z

test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/DefaultEditorTemplatesTest.cs

+        [InlineData("datetime", "datetime-local", null, "2000-01-02T03:04:05.006")]
+        [InlineData("datetime-local", "datetime-local", null, "2000-01-02T03:04:05.006")]
+        [InlineData("time", "time", "{0:t}", "03:04:05.006")]
+        public void Editor_FindsCorrectDateOrTimeTemplate(string dataTypeName, string expectedType, string editFormatString, string expected)


Move expectedType last or right after editFormatString.

dougbu · 2017-01-12T00:06:17Z

test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/DefaultEditorTemplatesTest.cs

+        [InlineData("datetime-local", "datetime-local", Html5DateRenderingMode.Rfc3339)]
+        [InlineData("time", "time", Html5DateRenderingMode.CurrentCulture)]
+        [InlineData("time", "time", Html5DateRenderingMode.Rfc3339)]
+        public void Editor_AppliesNonDefaultEditFormat(string dataTypeName, string expectedType, Html5DateRenderingMode renderingMode)


Move expectedType last.

jbagga · 2017-01-13T02:00:44Z

@dougbu Updated

Change datetime to datetime-local

bd40032

jbagga requested a review from dougbu January 6, 2017 19:57

dnfclas added the cla-already-signed label Jan 6, 2017

dougbu suggested changes Jan 6, 2017

View reviewed changes

Modify DefaultEditorTemplates and tests

48240d4

dougbu suggested changes Jan 10, 2017

View reviewed changes

Changed TemplateRenderer for DateTime

2742f14

Added tests

ee58ed8

dougbu suggested changes Jan 12, 2017

View reviewed changes

Added tests

455fb95

dougbu approved these changes Jan 13, 2017

View reviewed changes

jbagga merged commit f95d49c into dev Jan 13, 2017

jbagga deleted the jbagga/DateTimeLocal4871 branch January 14, 2017 00:55

DamianEdwards mentioned this pull request May 31, 2017

"Next Tuesday" helper button on Admin form doesn't work after updating to 2.0.0-preview* aspnet/live.asp.net#135

Closed

dougbu mentioned this pull request Aug 28, 2017

Input fields don't emit (and thus round-trip) offset value when binding against DateTimeOffset fields #6648

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change datetime to datetime-local #5664

Change datetime to datetime-local #5664

jbagga commented Jan 6, 2017

dougbu left a comment

dougbu Jan 6, 2017

jbagga commented Jan 10, 2017

dougbu Jan 10, 2017

dougbu Jan 10, 2017

dougbu Jan 10, 2017

Eilon Jan 10, 2017

dougbu Jan 10, 2017

dougbu Jan 10, 2017

dougbu Jan 10, 2017

Eilon Jan 10, 2017

dougbu Jan 10, 2017

dougbu Jan 10, 2017

jbagga Jan 10, 2017

dougbu Jan 10, 2017

jbagga commented Jan 10, 2017

dougbu commented Jan 11, 2017

jbagga commented Jan 11, 2017

dougbu Jan 11, 2017

dougbu Jan 11, 2017

dougbu Jan 12, 2017

dougbu Jan 12, 2017

dougbu Jan 12, 2017

jbagga commented Jan 13, 2017

		@@ -967,7 +969,9 @@ public void Process_GeneratesFormattedOutput(string specifiedType, string expect
		{
		{ "type", expectedType }

Change datetime to datetime-local #5664

Change datetime to datetime-local #5664

Conversation

jbagga commented Jan 6, 2017

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbagga commented Jan 10, 2017

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

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

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

jbagga commented Jan 10, 2017

dougbu commented Jan 11, 2017

jbagga commented Jan 11, 2017

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

Choose a reason for hiding this comment

jbagga commented Jan 13, 2017