Tests remaing field value in card view for null #419
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jholmes033169
changed the title
erquhart
suggested changes
May 15, 2017
example/config.yml
Outdated
| @@ -15,6 +15,7 @@ collections: # A list of collections the CMS should be able to edit | |||
| - {label: "Publish Date", name: "date", widget: "datetime", format: "YYYY-MM-DD hh:mma"} | |||
| - {label: "Cover Image", name: "image", widget: "image", required: false, tagname: ""} | |||
| - {label: "Body", name: "body", widget: "markdown"} | |||
| - {label: "Some Field", name: "somefield", widget: "string", required: false} # Testing empty field | |||
There was a problem hiding this comment.
We've been using the example case as a testbed more often, and I think it's an anti-pattern overall - let's remove the example changes and just implement the fix for now.
There was a problem hiding this comment.
I just removed the new test content that was in there... it was there for the other PR I did with the space in the image file name issue, and I just reused it to test this one. It's gone now.
There was a problem hiding this comment.
Can you remove this line?
| @@ -57,7 +57,7 @@ export default class EntryListing extends React.Component { | |||
| : inferedFields.remainingFields && inferedFields.remainingFields.map(f => ( | |||
| <p key={f.get('name')} className={styles.cardList}> | |||
| <span className={styles.cardListLabel}>{f.get('label')}:</span>{' '} | |||
| { entry.getIn(['data', f.get('name')], '').toString() } | |||
| { entry.getIn(['data', f.get('name')], '')?entry.getIn(['data', f.get('name')], '').toString():'' } | |||
There was a problem hiding this comment.
We can improve readability plus shorten a bit: (entry.getIn(['data', f.get('name')]) || '').toString().
|
LGTM |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
- Summary
In the current behavior, the CMS encounters a problem, while displaying "remaining fields" in a card, as it encounters a field with a value of "null" explicitly set in the front matter... blank works, but not null. The console errors with "Cannot read property 'toString' of null at EntryListing.js:60" and the CMS app will no longer respond until refreshed.
A null value occurs whenever a non-required field is skipped during post entry. This might not be intended behavior for persisting a post, but regardless, it should be handled.
The offending line originally read:
but now is changed to:
- Test plan
A post in the local content (index:1) was given a field of somefield: null and a corresponding field in the config was added to match it... and it was set required: false.
The app no longer crashes and all tests pass.
- Description for the changelog
Card remaining fields now are checked for null values.
- A picture of a cute animal (not mandatory but encouraged)