-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Widget fixes #162
Widget fixes #162
Conversation
|
||
DateTimeControl.propTypes = { | ||
onChange: PropTypes.func.isRequired, | ||
value: PropTypes.object, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PropTypes.object is not allowed in our linting rules, but i'm not sure there's any better choice for the DateControl or the DateTimeControl.
The problem here is that depending on the formatter
the value for a date control can either be a string, a Date or a moment.js object and the control (and preview) component should do the right thing with either of those.
|
||
DateControl.propTypes = { | ||
onChange: PropTypes.func.isRequired, | ||
value: PropTypes.object, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our linter treats PropTypes.object
as an error, but in this case I'm not sure there's a better option.
The DateControl (and preview) should be able to handle either a string, a Date or a moment.js object, since this will vary depending on the formatter (JSON and CSV for example, doesn't have any date concept and will deserialize date fields to a string).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a propType for that PropType.oneOfType
:)
Also, I'd suggest we don't couple with moment.js so tightly. So, I'd say:
PropTypes.oneOfType([
React.PropTypes.string,
React.PropTypes.instanceOf(Date)
])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, in case it is an object with the known shape, there is https://facebook.github.io/react/docs/react-api.html#react.proptypes.shape
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason we currently have a tighter coupling with moment is because in general it's so important to have timezone, date formating functions, etc, throughout the CMS that the standard Date model is not enough, and that the YAML formatter uses it as part of the serialization/deserializtion process right now as the only way to know how a date should be formattet in the YAML (for Jekyll for example, it makes a huge different if a date is persisted like 2016-11-01
or 2016-11-01 00:00:00
since once will get deserialized as a Ruby Date object, the other as a Ruby DateTime object and those behave differently).
The Ember prototype uses moment.js for deserialized dates since moment can carry around the format in the object - but I think we've actually broken that functionality in the React version right now...
- Summary
This ads a
date
widget for when date + time selection is not needed and also makesstring
the default widget when no widget is specified for a field in the config.yml- Description for the changelog
Add Date widget and make
widget
optional in field declarations