Feat input timeformat #16584

Narretz · 2018-05-30T19:23:13Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature

What is the current behavior? (You can also link to an open issue here)
Browser differences when displaying input time and datetime-local

What is the new behavior (if this is a feature change)?
Option to adjust the format

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

The commit message follows our guidelines
Fix/Feature: Docs have been added/updated
Fix/Feature: Tests have been added; existing tests pass

Other information:
Closes #10721
Closes #16510

There are two options:

timeFormatSeconds can be used to configure if seconds or milliseconds should be displayed in general.
timeStripEmptySeconds mimics the behavior of Chrome to hide the seconds completely if they are zero.

Narretz · 2018-05-30T19:24:17Z

src/ng/directive/input.js

+
+      if (isTimeType && isString(ctrl.$options.getOption('timeSecondsFormat'))) {
+        targetFormat = format.replace('ss.sss', ctrl.$options.getOption('timeSecondsFormat'));
+        if (targetFormat[targetFormat.length - 1] === ':') {


I know this is ugly but I couldn't be bothered to use a regex :D

It is ugly and would be simpler with:

targetFormat = targetFormat.replace(/:$/, '');

gkalpak

Generally LGTM.
I am not a big fun of the names tbh, though:

timeFormat: Not accurate, because the format does not apply to the whole time value.
timeSecondsFormat: Maybe confusing, because when I read "seconds" my mind goes to the two digits after minutes. But arguably, milliseconds are part of the "seconds". So, I can live with that 😁
timeStripEmptySeconds: Maybe confusing (see above). Also, what does "empty seconds" mean? (I know what it means, but only because I looked at the code 😁)

Question:
Why not expand formatting to minutes as well (e.g. strip "empty" minutes)? Was it an arbitrary decision? Is it to match the behavior of a popular browser?

gkalpak · 2018-05-31T11:56:21Z

src/ng/directive/input.js

+   * this is the timezone of the browser.
+   *
+   * The format of the displayed time can be adjusted with the
+   * {@link ng.directive:ngModelOptions#ngModelOptions-arguments ngModelOptions} `timeFormat` and


timeFormat --> timeSecondsFormat (?)

gkalpak · 2018-05-31T11:59:27Z

src/ng/directive/input.js

+      var formatted =  $filter('date')(value, targetFormat, timezone);
+
+      if (isTimeType && ctrl.$options.getOption('timeStripEmptySeconds')) {
+        formatted = formatted.replace(/(:00)?(\.000)?$/, '');


Nit: Since you are not using the captured groups, it is better to use (?:...) (instead of (...)).

gkalpak · 2018-05-31T12:01:07Z

src/ng/directive/input.js

@@ -255,6 +255,10 @@ var inputType = {
    * The timezone to be used to read/write the `Date` instance in the model can be defined using
    * {@link ng.directive:ngModelOptions ngModelOptions}. By default, this is the timezone of the browser.
    *
+    * The format of the displayed time can be adjusted with the
+    * {@link ng.directive:ngModelOptions#ngModelOptions-arguments ngModelOptions} `timeFormat` and


timeFormat --> timeSecondsFormat (?)

gkalpak · 2018-05-31T12:01:25Z

src/ng/directive/ngModelOptions.js

+ *
+ * ## Formatting the value of `time` and `datetime-local`
+ *
+ * With the options `timeFormat` and `timeStripEmptySeconds` it is possible to adjust the value


timeFormat --> timeSecondsFormat (?)

gkalpak · 2018-05-31T12:10:46Z

src/ng/directive/ngModelOptions.js

+          .component('timeExample', {
+            templateUrl: 'timeExample.html',
+            controller: function() {
+              this.time = {


Does this need to be an object?

No, but it's good practice ;)

_{Disagrees, but not strongly enough to be bothered 😛}

(it is only good practice if you are working directly with the scope - in this case you already have an object on the scope $ctrl so it doesn't matter)

gkalpak · 2018-05-31T12:11:43Z

src/ng/directive/ngModelOptions.js

+
+              this.optionChange = function() {
+                this.timeForm.timeFormatted.$overrideModelOptions(this.options);
+                this.time.value = new Date(this.time.value.getTime());


getTime() is redundant.

gkalpak · 2018-05-31T12:12:28Z

src/ng/directive/ngModelOptions.js

@@ -465,6 +527,21 @@ defaultModelOptions = new ModelOptions({
 *     Note that changing the timezone will have no effect on the current date, and is only applied after
 *     the next input / model change.
 *
+ *   - `timeFormat`: Defines if the `time` and `datetime-local` types should show seconds and


timeFormat --> timeSecondsFormat (?)

gkalpak · 2018-05-31T12:13:32Z

src/ng/directive/ngModelOptions.js

+ *     milliseconds. The option follows the format string of {@link date date filter}.
+ *     By default, the options is `undefined` which is equal to
+ *     `HH:mm:ss.sss` (shows hours, minutes, seconds and milliseconds). The other options are
+ *     `HH:mm:ss` (strips milliseconds), and `HH:mm`, which strips both seconds and milliseconds.


This description seems out-of-date.

gkalpak · 2018-05-31T12:26:42Z

test/ng/directive/inputSpec.js

+        $rootScope.time = new Date(1970, 0, 1, 15, 41, 50, 50);
+      });
+      expect(inputElm.val()).toBe('1970-01-01T15:41:50.050');
+


Unnecessary empty line 😱

gkalpak · 2018-05-31T12:26:57Z

test/ng/directive/inputSpec.js

+        $rootScope.time = new Date(1970, 0, 1, 15, 41, 50, 50);
+      });
+      expect(inputElm.val()).toBe('15:41:50.050');
+


Unnecessary empty line 😱

Narretz · 2018-05-31T12:53:11Z

Yeah the names aren't great. Since they are already verbose we could change it to "timeStripSecondsIfZero" or maybe "timeStripZeroSeconds".
I think since milliseconds are part of seconds "timeSecondsFormat" is fine. Looks like I did a sloppy job changing it everywhere.

Narretz · 2018-05-31T12:55:29Z

Oh, and why not include minutes: browsers that support time need hours and minutes and will also always display them even if they are 0.

petebacondarwin

Are you going to rename timeStripEmptySeconds?

petebacondarwin · 2018-06-06T12:13:05Z

src/ng/directive/input.js

+
+      if (isTimeType && isString(ctrl.$options.getOption('timeSecondsFormat'))) {
+        targetFormat = format.replace('ss.sss', ctrl.$options.getOption('timeSecondsFormat'));
+        if (targetFormat[targetFormat.length - 1] === ':') {


It is ugly and would be simpler with:

targetFormat = targetFormat.replace(/:$/, '');

petebacondarwin · 2018-06-06T12:22:43Z

src/ng/directive/ngModelOptions.js

+          .component('timeExample', {
+            templateUrl: 'timeExample.html',
+            controller: function() {
+              this.time = {


(it is only good practice if you are working directly with the scope - in this case you already have an object on the scope $ctrl so it doesn't matter)

Narretz · 2018-06-06T12:39:46Z

I want to rename it, but which is better - stripZeroSeconds or stripSecondsIfZero?

petebacondarwin · 2018-06-06T20:54:49Z

I preferstripZeroSeconds

Closes #10721 Closes #16510 Closes #16584

Narretz added 3 commits May 25, 2018 18:47

wip: time format

d6e7920

add option to datetime-local and add docs

a676fa5

change to timeSecondsFormat

0083aef

googlebot added the cla: yes label May 30, 2018

Narretz added type: feature component: forms frequency: high severity: confusing labels May 30, 2018

Narretz added this to the 1.7.x milestone May 30, 2018

Narretz added the needs: review label May 30, 2018

Narretz commented May 30, 2018

View reviewed changes

gkalpak suggested changes May 31, 2018

View reviewed changes

fixup! add option to datetime-local and add docs

1dea038

petebacondarwin approved these changes Jun 6, 2018

View reviewed changes

fixup! fixup! add option to datetime-local and add docs

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

11072f6

fixup! wip: time format

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

a528a70

Narretz merged commit 83f7980 into angular:master Jun 12, 2018

Narretz deleted the feat-input-timeformat branch June 12, 2018 21:45

Narretz added a commit that referenced this pull request Jun 13, 2018

feat(ngModelOptions): add timeStripZeroSeconds and timeSecondsFormat

b682213

Closes #10721 Closes #16510 Closes #16584

dependencies bot mentioned this pull request Aug 9, 2018

Update 6 dependencies from npm JetBrains/ring-ui#349

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat input timeformat #16584

Feat input timeformat #16584

Narretz commented May 30, 2018

Narretz May 30, 2018

petebacondarwin Jun 6, 2018

gkalpak left a comment

gkalpak May 31, 2018

gkalpak May 31, 2018

gkalpak May 31, 2018

gkalpak May 31, 2018

gkalpak May 31, 2018

Narretz Jun 1, 2018

gkalpak Jun 1, 2018

petebacondarwin Jun 6, 2018

gkalpak May 31, 2018

gkalpak May 31, 2018

gkalpak May 31, 2018

gkalpak May 31, 2018

gkalpak May 31, 2018

Narretz commented May 31, 2018

Narretz commented May 31, 2018

petebacondarwin left a comment

petebacondarwin Jun 6, 2018

petebacondarwin Jun 6, 2018

Narretz commented Jun 6, 2018

petebacondarwin commented Jun 6, 2018

Feat input timeformat #16584

Feat input timeformat #16584

Conversation

Narretz commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak 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

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

Narretz commented May 31, 2018

Narretz commented May 31, 2018

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz commented Jun 6, 2018

petebacondarwin commented Jun 6, 2018