Fix/improve date format #67
Conversation
| @@ -46,7 +47,7 @@ class DateTimeFormatPicker extends React.Component<Props, State> { | |||
| return null; | |||
| } | |||
| let expectedDateTime = attributes.find(attr => attr.name == attribute); | |||
| return expectedDateTime ? expectedDateTime.value : null; | |||
| return expectedDateTime ? expectedDateTime.value.trim() : null; | |||
diegoquinteiro
force-pushed
the
php-date-format
branch
from
a54d878
to
e597731
Compare
There was a problem hiding this comment.
Thanks so much for these changes, @mburak! The issue you are fixing is a show-stopper.
I have mixed feelings about replacing a standard format with the PHP one (I believe the SDK should be able to handle the ISO format instead) but given the importance of fixing the dates, I am fine with it.
I believe that before merging we should avoid adding new lint warnings, and I strongly recommend adding some tests to make sure it is clear that we are using the PHP date format.
| @@ -55,15 +56,20 @@ class DateTimeFormatPicker extends React.Component<Props, State> { | |||
| if (this.expectedDateTime() != null) { | |||
| m = moment.parseZone(this.expectedDateTime()); | |||
| if (m.isValid()) { | |||
There was a problem hiding this comment.
do we need to keep the call to isValid?
| @@ -26,7 +26,7 @@ export type JSONFormat = { | |||
|
|
|||
| type RulePropertyJSON = { | |||
| attribute: ?string, | |||
| dateTimeFormat?: string, | |||
| format?: string, | |||
There was a problem hiding this comment.
just out of curiosity, what triggered the change of name?
There was a problem hiding this comment.
This was a bug. This is the output format, and the SDK expects format. That explains why the preview never had the date =)
There was a problem hiding this comment.
I understand the SDK expects format, which was in fact what we were sending. I believe there was no need to change this name here, but it is not a show-stopper.
| * @flow | ||
| */ | ||
|
|
||
| (function(m) { |
There was a problem hiding this comment.
do you think there is a way to implement it by exporting as a module instead of as an iife?
| */ | ||
| const moment = require('moment'); | ||
|
|
||
| var formatMap = { |
There was a problem hiding this comment.
can we split the initialization here to avoid the lint warning?
| a: 'a', | ||
| A: 'A', | ||
| B: function() { | ||
| var thisUTC = this.clone().utc(), |
There was a problem hiding this comment.
can we also split the initialization here?
| @@ -11,13 +11,14 @@ | |||
| const React = require('react'); | |||
| const classNames = require('classnames'); | |||
| const moment = require('moment'); | |||
| const phpDateTimeMomentFormat = require('../utils/phpDateTimeMomentFormat'); | |||
There was a problem hiding this comment.
I am getting an unused var warning here. Is there a way to avoid it? I understand that you are adding a function, so you need to load the file.
|
@mburak I took the liberty to fix some of the linter warnings. Requesting now @diegoquinteiro to take a look. |
| @@ -26,7 +26,7 @@ export type JSONFormat = { | |||
|
|
|||
| type RulePropertyJSON = { | |||
| attribute: ?string, | |||
| dateTimeFormat?: string, | |||
| format?: string, | |||
There was a problem hiding this comment.
This was a bug. This is the output format, and the SDK expects format. That explains why the preview never had the date =)
src/js/utils/RuleExporter.js
Outdated
| ? 'textContent' | ||
| : 'innerContent', | ||
| format: rulePropertyJSON.dateTimeFormat, | ||
| rulePropertyJSON.type == RulePropertyTypes.DATETIME && |
There was a problem hiding this comment.
We cannot remove the logic for dealing with empty attributes in other fields. The logic here should be:
- If attribute came not null, use the attribute name.
- If the attribute came null:
- If the type is string, use 'textContent'
- If the type is date, use 'dateTextContent'
- Otherwise use 'innerContent'
diegoquinteiro
force-pushed
the
php-date-format
branch
from
383c9e0
to
f4f8999
Compare
This PR changes the date format we are using in the rules from ISO 8601 to PHP date format so it's compatible with the Facebook Instant Articles SDK. It also adds a link to the PHP Date documentation.