Fields default to Blank String #1126

MichaelRomani · 2018-02-21T20:58:34Z

Summary
Closes issue Fields default to null value #977
Widget values default to blank string when value is null.
Test plan
Values for title, image and body within redux store get set to '' when value is null (date currently being set to current date)
Description for the changelog
Default values directly provided by each widget.

verythorough · 2018-02-21T21:00:32Z

Deploy preview for cms-demo ready!

Built with commit e8945fb

https://deploy-preview-1126--cms-demo.netlify.com

verythorough · 2018-02-21T21:00:59Z

Deploy preview for netlify-cms-www ready!

Built with commit e8945fb

https://deploy-preview-1126--netlify-cms-www.netlify.com

erquhart · 2018-02-22T18:20:57Z

This approach works, but I'd like to see a less involved method for handling defaults since every widget will need to tackle this concern.

Right now, we're intentionally setting empty fields to null: https://github.com/netlify/netlify-cms/blob/b4b584682473924556a41cae10f64f085b0f432b/src/actions/entries.js#L261-L263

A null value represents "the intentional absence of any object value", while undefined represents a value that simply hasn't been set (spec). The latter is a more accurate description of fields that have no default value, so I propose we remove the default setting of null, allowing empty fields to return undefined instead.

In doing so, we gain the ability to use React's defaultProps to set empty values in widgets (they only work with undefined values).

Thoughts?

MichaelRomani · 2018-02-22T20:03:00Z

Shawn, just to clarify, you'd like me to set a default for 'value' using defaultProps for each of the editor widgets that has 'value' within its propTypes object. Is this correct? Also, should I default each to an empty string including objects that might ultimately be set to a date / boolean?

erquhart · 2018-02-23T18:18:07Z

Each value will either be a number, string, boolean, array, object, or an array/object containing these types. Dates, for example, are stringified by their widget on change. Most widgets will use an empty string, with the exception of the boolean, object, and list widget. Perhaps there are others, those are the only ones that come to mind at the moment.

I'd expect Boolean to default to false, object to an empty object, and array to an empty array. But again, that's just off the top of my head without testing.

MichaelRomani · 2018-02-26T22:49:43Z

Thanks, I will make the adjustments and submit early this week!
-Mike

MichaelRomani · 2018-02-27T21:51:28Z

I've updated the PR to include 'defaultProps' for values located within the editor widget files. I did not set the default for the date/time as the current set up will only set the current date / time if value is not equal to an empty string. I also updated the createEmptyDraft function to set all values as undefined in place of null.

tech4him1 · 2018-02-27T22:14:39Z

src/actions/entries.js

@@ -259,7 +259,7 @@ export function createEmptyDraft(collection) {
  return (dispatch) => {
    const dataFields = {};
    collection.get('fields', List()).forEach((field) => {
-      dataFields[field.get('name')] = field.get('default', null);
+      dataFields[field.get('name')] = undefined;


@MichaelRomani We still want to allow users to manually set a default here, just default it to undefined instead of null. The code would be field.get('default'), just removing null as the second param like @erquhart mentioned.

MichaelRomani · 2018-02-27T22:27:45Z

@tech4him1 - I've made the adjustment, setting it equal to - field.get('default').

erquhart · 2018-02-27T22:33:00Z

Getting close!

The ultimate test is creating a new Kitchen Sink entry in the demo - currently failing: https://deploy-preview-1126--cms-demo.netlify.com/#/collections/kitchenSink

tech4him1 · 2018-02-27T22:29:56Z

src/components/EditorWidgets/Markdown/MarkdownPreview/index.js

@@ -15,4 +15,8 @@ MarkdownPreview.propTypes = {
  value: PropTypes.string,
 };

+MarkdownPreview.defaultProps = {
+  value: '',


Is there a reason we need to set this on the MD preview, and not any of the other previews?

Are you referring to the 'RawEditor' and 'VisualEditor' files?

As far as the kitchen sink goes, I'll take a look this evening or tomorrow afternoon. My apologies, I did not test creating a new sink.

No, I just noticed that you set a default value on the MarkdownPreview as well as the MarkdownControl. Was there a reason?

tech4him1 · 2018-02-27T22:35:49Z

@MichaelRomani Maybe I missed it, but is there a reason a default isn't set for the ListControl?

tech4him1 · 2018-02-27T22:40:47Z

@erquhart Is changing the default from null to undefined going to be a breaking change?

erquhart · 2018-02-28T01:09:56Z

It would only be breaking for an installation if someone were intentionally checking for that null value in an external custom widget, in which case, the null value isn't a part of our published API, so it's not technically a breaking change. I also think the likelihood of real world problems resulting is very low, but I'm quite open to being told I'm wrong 😄.

MichaelRomani · 2018-02-28T03:21:05Z

ListControl - A defaultProp for value was already set in this file as:
value: List()

MarkdownPreview - I removed the defaultProps that had been added to this file. I mistakenly thought it was receiving 'undefined' and needed to be set to ''.

Kitchen Sink Error -The error seems to have been coming from ObjectControl. I think the issue is that the defaultProp should have been set to a Map Object as:
Map().
Locally, Kitchen Sink is now working.

Let me know if this makes sense and I'll push these changes up to this PR for review.

erquhart · 2018-02-28T04:39:41Z

Sounds good, push it up!

tech4him1 · 2018-02-28T16:55:57Z

@MichaelRomani Great, thanks!

into issue-977

MichaelRomani · 2018-02-28T21:28:06Z

I've updated the PR. Let me know if changes are needed.

erquhart · 2018-03-02T22:28:03Z

Any thoughts from anyone about the breaking change concern?

cc/ @tech4him1

tech4him1 · 2018-03-02T22:56:43Z

@erquhart On #1126, does the CMS internally treat null differently than undefined? Maybe by writing a key as empty vs not writing at all, or anything like that?

If not, I don't see a big concern with backwards compatibility.

erquhart · 2018-03-02T23:10:08Z

Not that I'm aware of. If bugs emerge we should submit fixes with test coverage, but I haven't seen any issues.

tech4him1

I think this should be documented at some point for widget authors, but the PR looks good to me!

withMedia, StringControl, visualEditor - edits

ba1a709

MichaelRomani added 3 commits February 27, 2018 16:37

changes

1d7c1a9

issue-977

a19b930

Merge branch 'master' into issue-977

9f7084d

tech4him1 reviewed Feb 27, 2018

View reviewed changes

MichaelRomani added 3 commits February 27, 2018 17:20

createEmptyDraft adjusted

c058e4b

createEmptyDraft adjusted

440efcd

adjusted

e844d7c

tech4him1 reviewed Feb 27, 2018

View reviewed changes

tech4him1 closed this Feb 27, 2018

tech4him1 reopened this Feb 27, 2018

MichaelRomani and others added 4 commits February 28, 2018 13:09

changes

e192bcd

Update index.js

4f59946

Merge branch 'issue-977' of https://github.com/MichaelRomani/netlify-cms

a684cd2

into issue-977

fix issue w/accidental line erasures

e8945fb

erquhart approved these changes Mar 19, 2018

View reviewed changes

erquhart added impact: changelog area: extensions/widgets area: configuration status: 2-code-review labels Mar 19, 2018

tech4him1 self-requested a review March 21, 2018 23:03

tech4him1 approved these changes Mar 27, 2018

View reviewed changes

erquhart merged commit 55f01e6 into decaporg:master Mar 28, 2018

tech4him1 mentioned this pull request Mar 29, 2018

Fields default to null value #977

Closed

brianlmacdonald pushed a commit to brianlmacdonald/netlify-cms that referenced this pull request May 23, 2018

Fields default to Blank String (decaporg#1126)

cf9ae70

papandreou mentioned this pull request May 24, 2018

Image alt text in markdown widget defaults to null #956

Closed

tech4him1 mentioned this pull request Jun 18, 2018

Values from string and markdown widgets come out as either empty string or undefined, depending on whether the field has been edited #1449

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fields default to Blank String #1126

Fields default to Blank String #1126

MichaelRomani commented Feb 21, 2018

verythorough commented Feb 21, 2018 •

edited

Loading

verythorough commented Feb 21, 2018 •

edited

Loading

erquhart commented Feb 22, 2018 •

edited

Loading

MichaelRomani commented Feb 22, 2018

erquhart commented Feb 23, 2018 •

edited

Loading

MichaelRomani commented Feb 26, 2018

MichaelRomani commented Feb 27, 2018

tech4him1 Feb 27, 2018

MichaelRomani commented Feb 27, 2018

erquhart commented Feb 27, 2018 •

edited

Loading

tech4him1 Feb 27, 2018

MichaelRomani Feb 27, 2018

MichaelRomani Feb 27, 2018

tech4him1 Feb 27, 2018 •

edited

Loading

tech4him1 commented Feb 27, 2018

tech4him1 commented Feb 27, 2018

erquhart commented Feb 28, 2018

MichaelRomani commented Feb 28, 2018

erquhart commented Feb 28, 2018

tech4him1 commented Feb 28, 2018

MichaelRomani commented Feb 28, 2018

erquhart commented Mar 2, 2018

tech4him1 commented Mar 2, 2018

erquhart commented Mar 2, 2018 •

edited

Loading

tech4him1 left a comment

Fields default to Blank String #1126

Fields default to Blank String #1126

Conversation

MichaelRomani commented Feb 21, 2018

verythorough commented Feb 21, 2018 • edited Loading

verythorough commented Feb 21, 2018 • edited Loading

erquhart commented Feb 22, 2018 • edited Loading

MichaelRomani commented Feb 22, 2018

erquhart commented Feb 23, 2018 • edited Loading

MichaelRomani commented Feb 26, 2018

MichaelRomani commented Feb 27, 2018

tech4him1 Feb 27, 2018

Choose a reason for hiding this comment

MichaelRomani commented Feb 27, 2018

erquhart commented Feb 27, 2018 • edited Loading

tech4him1 Feb 27, 2018

Choose a reason for hiding this comment

MichaelRomani Feb 27, 2018

Choose a reason for hiding this comment

MichaelRomani Feb 27, 2018

Choose a reason for hiding this comment

tech4him1 Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

tech4him1 commented Feb 27, 2018

tech4him1 commented Feb 27, 2018

erquhart commented Feb 28, 2018

MichaelRomani commented Feb 28, 2018

erquhart commented Feb 28, 2018

tech4him1 commented Feb 28, 2018

MichaelRomani commented Feb 28, 2018

erquhart commented Mar 2, 2018

tech4him1 commented Mar 2, 2018

erquhart commented Mar 2, 2018 • edited Loading

tech4him1 left a comment

Choose a reason for hiding this comment

verythorough commented Feb 21, 2018 •

edited

Loading

verythorough commented Feb 21, 2018 •

edited

Loading

erquhart commented Feb 22, 2018 •

edited

Loading

erquhart commented Feb 23, 2018 •

edited

Loading

erquhart commented Feb 27, 2018 •

edited

Loading

tech4him1 Feb 27, 2018 •

edited

Loading

erquhart commented Mar 2, 2018 •

edited

Loading