Datetime format #842

biilmann · 2017-11-24T06:49:09Z

An earlier change broke the format attribute for date and datetime widgets (see #529).

Before this was handled by always treating the value as a moment object and having the YAML serializer respect that.

This takes a slightly different approach and always treats the value of date and datetime widgets as a formatted string. If we take this approach we should get rid of the moment specific code for formats in the YAML serializer.

The downside of this is that some future supported formats might handle actual date/datetime objects - but it seems this is the only approach I can think of where the format attribute would be respected across any output format apart from that...

tech4him1 · 2017-11-25T15:22:41Z

@biilmann I'm missing some context here -- Why did we make the change that broke it, in the first place?

biilmann · 2017-11-26T19:20:17Z

I did a little digging, and it seems we never actually supported the format attribute in the React version of the CMS. Here's some comments around it in my initial PR adding the widget:

https://github.com/netlify/netlify-cms/pull/162/files#r88311221

In the older Ember version, this was handled by always turning the value into a moment object. Moment keeps track of the formatting set for a date in an internal ._f attribute, and we made the YAML serializer take that into account here:

https://github.com/netlify/netlify-cms/blob/master/src/formats/yaml.js#L6

I think the approach suggested here - of simply treating dates as formatted strings - is going to be more robust in general (since most of the supported formats have no standard for dates). If keeping the existing behavior feels important, we could make sure that we always keep the entry value as a date when no format attribute is present...

biilmann · 2017-11-26T19:32:35Z

I updated the PR to keep the current behavior when no format attribute is set.

tech4him1 · 2017-11-27T16:32:18Z

I'm fine with a formatted string (even with no fallback), it just seems way simpler, like you said. I really don't see any reason why we would need an actual Date/Moment object instead of a string, unless there was a valid use case for that before.

erquhart · 2017-11-27T21:38:18Z

@Benaiah per our convo, make the updates you'll be mentioning in your review, handle any necessary rebasing, and merge as soon as you can.

Benaiah

I'm preparing a commit to fix the specific issues I noted in the review comments.

This changes do seem to work on save (the PR is currently broken but renaming the components correctly makes it work), but it still has a couple shortcomings (similar shortcomings are why I haven't submitted a PR for this issue yet).

The DateTime control we're using supports passing a format setting to control visible controls and the display of the time controls (the two settings are conflated in the component, unfortunately). This would be work well, except that it splits the timeFormat and dateFormat for no apparent reason, forcing us to either use the default UI or expose that bad API to the user. In my attempt at fixing this issue, I used only dateFormat (ignoring timeFormat) and tried to work around the issues caused by the date/time distinction and the conflation of multiple different settings into the *Format` props. This proved difficult to do.

The problem with that approach was that when you had a format that didn't save a piece of data you needed, it would immediately reset to the default value, which made the UI look very broken. In addition, the displayed date was not formatted correctly. (Passing the full format to dateFormat resulted in the correct display in the string input except that the time was duplicated - I couldn't set the timeFormat to empty to fix it because that hides the time controls).

This PR does the opposite - it ignores the component's format props for purposes of control and display, choosing instead to only format on save. This has similar, but slightly different issues. Controls are similarly not hidden correctly when they are setting a value not present in the format, but they break differently. In addition, the time shown in the string input of the date does not represent the data actually saved.

IMO the problem will be best fixed by fixing the underlying control. I'm ok with merging this without a fix to that issue, since the PR is an improvement, I just wanted to point out the issue and the underlying cause.

Benaiah · 2017-11-27T18:10:12Z

src/components/Widgets/DateControl.js

+  return value;
+}
+
+export default class DateTimeControl extends React.Component {


This naming is very confusing (there's two DateTimeControls, one of which is in DateTimeControl.js and one of which is in DateControl.js.

Yes, it's an old oversight predating this PR, but obviously the DateControl should just export DateControl

Benaiah · 2017-11-27T18:10:52Z

src/components/Widgets/DateTimeControl.js

 import React from 'react';
-import DateTime from 'react-datetime';
+import DateControl from './DateControl';


DateControl.js as written exports DateTimeControl, not DateControl. (This is the cause of the PR being currently broken).

@Benaiah That's weird, a default export/import shouldn't break things if it's named differently. Am I mistaken there?

@tech4him1 it does seem like a default export should work regardless, but changing the name in DateControl.js fixed the broken CMS. (note: this is now fixed, even though this line hasn't actually changed).

Benaiah · 2017-11-27T20:31:52Z

src/components/Widgets/DateControl.js

-      onChange={this.handleChange}
+    return (
+      <DateTime
+        timeFormat={this.props.includeTime || false}


Using || with a literal false is logically redundant. This line is exactly equivalent to timeFormat={this.props.includeTime}.

Actually I think passing undefined or null to the timeFormat property will trigger the default setting which is true.

@biilmann ah - I'll add !! to coerce this to a boolean.

Benaiah · 2017-11-27T20:35:54Z

src/components/Widgets/DateControl.js

-export default class DateControl extends React.Component {
+function format(format, value) {
+  if (format) {
+    return moment(value).format(format || moment.defaultFormat);


The default of moment will never be reached here, since this is wrapped in if (format). A falsy format will never reach this branch, so this line is exactly equivalent to return moment(value).format(format).

yeah, that's an oversight in the commit to preserve the current behavior (which I do think makes sense for serialization formats that actually handle date objects)

Benaiah · 2017-11-27T21:19:09Z

src/components/Widgets/DateControl.js


-export default class DateControl extends React.Component {
+function format(format, value) {


I'm not a fan of the interface of format and toDate - depending on the falsiness of one argument they:

either do something or do nothing

either return either a specific type of value or return whatever was passed in

The name of the format function is also very confusing, as it collides with the format variable frequently used in this file.

Benaiah · 2017-11-27T22:35:14Z

@erquhart 👍 as noted in my review, I'm working on a commit to fix these and there's no need for @biilmann to address them himself.

Benaiah · 2017-11-27T23:02:33Z

I've now updated the PR with fixes to the problems I mentioned.

~~EDIT: one last boolean coercion to add~~

Added boolean coercion.

erquhart · 2017-11-28T02:18:26Z

@Benaiah so many people have complained about the empty date thing, figured we'd tack that on. Please give another review and merge if you have no objections.

biilmann requested review from erquhart and tech4him1 November 25, 2017 08:17

Benaiah suggested changes Nov 27, 2017

View reviewed changes

erquhart approved these changes Nov 28, 2017

View reviewed changes

biilmann and others added 4 commits November 27, 2017 20:52

Make date and datetime widget respect format attribute

fa396c6

Keep old behevior if no format attribute is set on date widgets

257ad3d

Fixes for date formatting PR

47ab2f5

Coerce includeTime to a boolean

54a617c

erquhart force-pushed the datetime-format branch from d28ff8c to 54a617c Compare November 28, 2017 01:53

simplify date time initial value set

c87660d

erquhart force-pushed the datetime-format branch from d6aabfb to 14f8bc9 Compare November 28, 2017 02:25

allow empty default date value

4c35b66

erquhart force-pushed the datetime-format branch from 14f8bc9 to 4c35b66 Compare November 28, 2017 03:16

erquhart merged commit a7ad6d5 into master Nov 28, 2017

erquhart deleted the datetime-format branch November 28, 2017 03:25

erquhart mentioned this pull request Nov 28, 2017

Allow setting an empty default date(time). #645

Closed

Benaiah mentioned this pull request Nov 28, 2017

datetime widget format is not respected #529

Closed

erquhart mentioned this pull request Dec 14, 2017

Add "empty" field to date and datetime widgets. #833

Closed

snyk-bot mentioned this pull request Dec 6, 2022

[Snyk] Security upgrade simple-git from 2.48.0 to 3.15.0 B3EF/netlify-cms#28

Open

mdno mentioned this pull request Apr 15, 2024

[Snyk] Fix for 1 vulnerabilities mdno/netlify-cms#103

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Datetime format #842

Datetime format #842

biilmann commented Nov 24, 2017

tech4him1 commented Nov 25, 2017

biilmann commented Nov 26, 2017

biilmann commented Nov 26, 2017

tech4him1 commented Nov 27, 2017

erquhart commented Nov 27, 2017 •

edited

Loading

Benaiah left a comment

Benaiah Nov 27, 2017

biilmann Nov 27, 2017

Benaiah Nov 27, 2017

tech4him1 Nov 27, 2017 •

edited

Loading

biilmann Nov 27, 2017

Benaiah Nov 27, 2017

Benaiah Nov 27, 2017

biilmann Nov 27, 2017

Benaiah Nov 27, 2017

Benaiah Nov 27, 2017

biilmann Nov 27, 2017

Benaiah Nov 27, 2017 •

edited

Loading

Benaiah commented Nov 27, 2017 •

edited

Loading

Benaiah commented Nov 27, 2017 •

edited

Loading

erquhart commented Nov 28, 2017


		export default class DateControl extends React.Component {
		function format(format, value) {

Datetime format #842

Datetime format #842

Conversation

biilmann commented Nov 24, 2017

tech4him1 commented Nov 25, 2017

biilmann commented Nov 26, 2017

biilmann commented Nov 26, 2017

tech4him1 commented Nov 27, 2017

erquhart commented Nov 27, 2017 • edited Loading

Benaiah 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

tech4him1 Nov 27, 2017 • edited Loading

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

Benaiah Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Benaiah commented Nov 27, 2017 • edited Loading

Benaiah commented Nov 27, 2017 • edited Loading

erquhart commented Nov 28, 2017

erquhart commented Nov 27, 2017 •

edited

Loading

tech4him1 Nov 27, 2017 •

edited

Loading

Benaiah Nov 27, 2017 •

edited

Loading

Benaiah commented Nov 27, 2017 •

edited

Loading

Benaiah commented Nov 27, 2017 •

edited

Loading