Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix frontmatter stringification for date fields #384

Closed
wants to merge 3 commits into from

Conversation

erquhart
Copy link
Contributor

Switches back to custom frontmatter stringification
until support lands in the preliminaries lib. This is necessary
because we use custom schemas for certain data types,
such as dates and times.

- Summary

Date fields can no longer be changed through the CMS. Regression from #348.

- Test plan

Moved the Published Date field in the example project out of metadata and into standard fields, so persisting dates can be observed in deploy previews. Tested locally.

- Description for the changelog

Fix date/datetime field persisting

Switches back to custom frontmatter stringification
until support lands in preliminaries. This is necessary
because we use custom schemas for certain data types,
such as dates and times.
@erquhart erquhart requested review from josephearl and Benaiah April 20, 2017 19:47
Copy link
Contributor

@Benaiah Benaiah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and fixes the issue on my test repo.

EDIT: looks like a test needs to be updated; the Travis build is failing.

@josephearl
Copy link
Contributor

👍 good idea

@josephearl
Copy link
Contributor

josephearl commented Apr 20, 2017

Another option is to pass a custom parser implementation that wraps the existing YAML to preliminaries:

var parser = {
  stringify: function(meta) {
    return new YAML().toFile(meta, sortedKeys)
  }
}
return preliminaries.stringify(body, meta, { lang: 'yaml', delims: '---', parser });

@calavera
Copy link
Contributor

I like @josephearl's suggestion.

@Benaiah
Copy link
Contributor

Benaiah commented Apr 20, 2017

@erquhart @calavera @josephearl I have the tests passing (mocked src/valueObjects/AssetProxy.js and updated a stringify test which had slightly different whitespace). Once the test changes are reviewed it should be ready to merge.

@calavera
Copy link
Contributor

Thanks @Benaiah.

LGTM

@erquhart
Copy link
Contributor Author

@josephearl can you update to use our custom parser with preliminaries? Otherwise I will when I get back online.

@Benaiah Benaiah mentioned this pull request Apr 20, 2017
@Benaiah
Copy link
Contributor

Benaiah commented Apr 20, 2017

Closing in favor of #385, which uses the implementation suggested by @josephearl.

@Benaiah Benaiah closed this Apr 20, 2017
@calavera calavera deleted the fix-date-field-persist branch April 20, 2017 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants