GitHub PullRequestReviewCommentEvent at 2014-08-21 05:29:20
Repository: keystone
Actor: JedWatson
Payload: {"action":"created","comment":{"url":"https://api.github.com/repos/JedWatson/keystone/pulls/comments/16521138","id":16521138,"diff_hunk":"@@ -0,0 +1,101 @@\n+/** @jsx React.DOM */\n+\n+var React = require('react'),\n+\tField = require('../field'),\n+\tpikaday = require('pikaday'),\n+\tmoment = require('moment');\n+\n+module.exports = Field.create({\n+\n+\tsupports: {\n+\t\tfocusTarget: 'dateInput'\n+\t},\n+\n+\t// default formats\n+\tinputFormat: 'YYYY-MM-DD',\n+\tnoEditFormat: 'Do MMM YYYY',\n+\n+\tgetInitialState: function() {\n+\t\treturn { \n+\t\t\tvalue: this.props.value ? moment(this.props.value).format(this.inputFormat) : ''\n+\t\t};\n+\t},\n+\n+\tcomponentDidMount: function() {\n+\t\tvar _this = this;","path":"fields/types/date/component.js","position":25,"original_position":25,"commit_id":"2b01cf5bf709a2e278aba9f70a6baa702c77e4d0","original_commit_id":"2b01cf5bf709a2e278aba9f70a6baa702c77e4d0","user":{"login":"JedWatson","id":872310,"avatar_url":"https://avatars.githubusercontent.com/u/872310?v=2","gravatar_id":"6a5996e5c80339e7457d77b7162baacf","url":"https://api.github.com/users/JedWatson","html_url":"https://github.com/JedWatson","followers_url":"https://api.github.com/users/JedWatson/followers","following_url":"https://api.github.com/users/JedWatson/following{/other_user}","gists_url":"https://api.github.com/users/JedWatson/gists{/gist_id}","starred_url":"https://api.github.com/users/JedWatson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/JedWatson/subscriptions","organizations_url":"https://api.github.com/users/JedWatson/orgs","repos_url":"https://api.github.com/users/JedWatson/repos","events_url":"https://api.github.com/users/JedWatson/events{/privacy}","received_events_url":"https://api.github.com/users/JedWatson/received_events","type":"User","site_admin":false},"body":"I've been using function binding in the other react components, would be good to stay consistent. Is there a strong case for enclosed variables being the pattern we use?\r\n\r\nIf we want to go with closure references, I tend to use either `self` or a descriptive variable name (e.g. `component`) in node-side Keystone code, again would like to keep consistency.","created_at":"2014-08-21T05:29:20Z","updated_at":"2014-08-21T05:29:20Z","html_url":"https://github.com/JedWatson/keystone/pull/559#discussion_r16521138","pull_request_url":"https://api.github.com/repos/JedWatson/keystone/pulls/559","_links":{"self":{"href":"https://api.github.com/repos/JedWatson/keystone/pulls/comments/16521138"},"html":{"href":"https://github.com/JedWatson/keystone/pull/559#discussion_r16521138"},"pull_request":{"href":"https://api.github.com/repos/JedWatson/keystone/pulls/559"}}},"pull_request":{"url":"https://api.github.com/repos/JedWatson/keystone/pulls/559","id":20088872,"html_url":"https://github.com/JedWatson/keystone/pull/559","diff_url":"https://github.com/JedWatson/keystone/pull/559.diff","patch_url":"https://github.com/JedWatson/keystone/pull/559.patch","issue_url":"https://api.github.com/repos/JedWatson/keystone/issues/559","number":559,"state":"open","locked":false,"title":"Adding date type react component","user":{"login":"JohnnyEstilles","id":5514878,"avatar_url":"https://avatars.githubusercontent.com/u/5514878?v=2","gravatar_id":"5b42d2c45dbe06521257b3e9752a8a5d","url":"https://api.github.com/users/JohnnyEstilles","html_url":"https://github.com/JohnnyEstilles","followers_url":"https://api.github.com/users/JohnnyEstilles/followers","following_url":"https://api.github.com/users/JohnnyEstilles/following{/other_user}","gists_url":"https://api.github.com/users/JohnnyEstilles/gists{/gist_id}","starred_url":"https://api.github.com/users/JohnnyEstilles/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/JohnnyEstilles/subscriptions","organizations_url":"https://api.github.com/users/JohnnyEstilles/orgs","repos_url":"https://api.github.com/users/JohnnyEstilles/repos","events_url":"https://api.github.com/users/JohnnyEstilles/events{/privacy}","received_events_url":"https://api.github.com/users/JohnnyEstilles/received_events","type":"User","site_admin":false},"body":"This date component is functionally equivalent to its \"pre-react\" counterpart.\r\n\r\nI did not add any client-side validation yet because I believe we should come to a consensus on how we will implement the client/server-side code sharing.\r\n\r\nOn the server-side code, I added a `formatString` property to store the string passed using the `format` option. This way the format string is available to the client-side component. I didn't name the property \"format\" because it was already used by a method. As per a discussion with @JedWatson on Slack, when `noedit` is true and the `format` option is used, the `format` string is now used to display the date field value. However, it will default to the `Do MMM YYYY` format when no `format` option is used.\r\n\r\nI've been holding off on suggesting a couple tweaks to the date field functionality until we started the move to react, so I guess now is the time.\r\n\r\n1. Use strict parsing with moment() so that calls with incomplete dates, such as `moment('1', 'YYYY-MM-DD')`, will not pass validation. It's kind of weird that enter `1` renders  a `2001-01-01` date.\r\n2. Add an optional feature that will prevent user input and force selection from date picker, and another that ONLY shows the date picker. (My customer's have been requesting these for specific use cases.)\r\n\r\nAlso, I noticed we're getting a `moment()` deprecation warning on the server-side `updateItem()` method for the date field. \r\n\r\n> Deprecation warning: moment construction falls back to js Date. This is discouraged and will be removed in upcoming major release. Please refer to https://github.com/moment/moment/issues/1407 for more info.\r\n\r\nMoment now requires the format argument when parsing a date.I will clean that up on my next PR.\r\n\r\nLet me know what you all think about my suggested tweaks and let's talk about client-side validation (I think it's long overdue).\r\n","created_at":"2014-08-21T04:13:35Z","updated_at":"2014-08-21T05:29:20Z","closed_at":null,"merged_at":null,"merge_commit_sha":"0bc0945b362ff24559a30970d538316d159ca958","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/JedWatson/keystone/pulls/559/commits","review_comments_url":"https://api.github.com/repos/JedWatson/keystone/pulls/559/comments","review_comment_url":"https://api.github.com/repos/JedWatson/keystone/pulls/comments/{number}","comments_url":"https://api.github.com/repos/JedWatson/keystone/issues/559/comments","statuses_url":"https://api.github.com/repos/JedWatson/keystone/statuses/2b01cf5bf709a2e278aba9f70a6baa702c77e4d0","head":{"label":"JohnnyEstilles:field-type-date","ref":"field-type-date","sha":"2b01cf5bf709a2e278aba9f70a6baa702c77e4d0","user":{"login":"JohnnyEstilles","id":5514878,"avatar_url":"https://avatars.githubusercontent.com/u/5514878?v=2","gravatar_id":"5b42d2c45dbe06521257b3e9752a8a5d","url":"https://api.github.com/users/JohnnyEstilles","html_url":"https://github.com/JohnnyEstilles","followers_url":"https://api.github.com/users/JohnnyEstilles/followers","following_url":"https://api.github.com/users/JohnnyEstilles/following{/other_user}","gists_url":"https://api.github.com/users/JohnnyEstilles/gists{/gist_id}","starred_url":"https://api.github.com/users/JohnnyEstilles/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/JohnnyEstilles/subscriptions","organizations_url":"https://api.github.com/users/JohnnyEstilles/orgs","repos_url":"https://api.github.com/users/JohnnyEstilles/repos","events_url":"https://api.github.com/users/JohnnyEstilles/events{/privacy}","received_events_url":"https://api.github.com/users/JohnnyEstilles/received_events","type":"User","site_admin":false},"repo":{"id":21588637,"name":"keystone","full_name":"JohnnyEstilles/keystone","owner":{"login":"JohnnyEstilles","id":5514878,"avatar_url":"https://avatars.githubusercontent.com/u/5514878?v=2","gravatar_id":"5b42d2c45dbe06521257b3e9752a8a5d","url":"https://api.github.com/users/JohnnyEstilles","html_url":"https://github.com/JohnnyEstilles","followers_url":"https://api.github.com/users/JohnnyEstilles/followers","following_url":"https://api.github.com/users/JohnnyEstilles/following{/other_user}","gists_url":"https://api.github.com/users/JohnnyEstilles/gists{/gist_id}","starred_url":"https://api.github.com/users/JohnnyEstilles/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/JohnnyEstilles/subscriptions","organizations_url":"https://api.github.com/users/JohnnyEstilles/orgs","repos_url":"https://api.github.com/users/JohnnyEstilles/repos","events_url":"https://api.github.com/users/JohnnyEstilles/events{/privacy}","received_events_url":"https://api.github.com/users/JohnnyEstilles/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/JohnnyEstilles/keystone","description":"node.js cms and web app framework","fork":true,"url":"https://api.github.com/repos/JohnnyEstilles/keystone","forks_url":"https://api.github.com/repos/JohnnyEstilles/keystone/forks","keys_url":"https://api.github.com/repos/JohnnyEstilles/keystone/keys{/key_id}","collaborators_url":"https://api.github.com/repos/JohnnyEstilles/keystone/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/JohnnyEstilles/keystone/teams","hooks_url":"https://api.github.com/repos/JohnnyEstilles/keystone/hooks","issue_events_url":"https://api.github.com/repos/JohnnyEstilles/keystone/issues/events{/number}","events_url":"https://api.github.com/repos/JohnnyEstilles/keystone/events","assignees_url":"https://api.github.com/repos/JohnnyEstilles/keystone/assignees{/user}","branches_url":"https://api.github.com/repos/JohnnyEstilles/keystone/branches{/branch}","tags_url":"https://api.github.com/repos/JohnnyEstilles/keystone/tags","blobs_url":"https://api.github.com/repos/JohnnyEstilles/keystone/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/JohnnyEstilles/keystone/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/JohnnyEstilles/keystone/git/refs{/sha}","trees_url":"https://api.github.com/repos/JohnnyEstilles/keystone/git/trees{/sha}","statuses_url":"https://api.github.com/repos/JohnnyEstilles/keystone/statuses/{sha}","languages_url":"https://api.github.com/repos/JohnnyEstilles/keystone/languages","stargazers_url":"https://api.github.com/repos/JohnnyEstilles/keystone/stargazers","contributors_url":"https://api.github.com/repos/JohnnyEstilles/keystone/contributors","subscribers_url":"https://api.github.com/repos/JohnnyEstilles/keystone/subscribers","subscription_url":"https://api.github.com/repos/JohnnyEstilles/keystone/subscription","commits_url":"https://api.github.com/repos/JohnnyEstilles/keystone/commits{/sha}","git_commits_url":"https://api.github.com/repos/JohnnyEstilles/keystone/git/commits{/sha}","comments_url":"https://api.github.com/repos/JohnnyEstilles/keystone/comments{/number}","issue_comment_url":"https://api.github.com/repos/JohnnyEstilles/keystone/issues/comments/{number}","contents_url":"https://api.github.com/repos/JohnnyEstilles/keystone/contents/{+path}","compare_url":"https://api.github.com/repos/JohnnyEstilles/keystone/compare/{base}...{head}","merges_url":"https://api.github.com/repos/JohnnyEstilles/keystone/merges","archive_url":"https://api.github.com/repos/JohnnyEstilles/keystone/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/JohnnyEstilles/keystone/downloads","issues_url":"https://api.github.com/repos/JohnnyEstilles/keystone/issues{/number}","pulls_url":"https://api.github.com/repos/JohnnyEstilles/keystone/pulls{/number}","milestones_url":"https://api.github.com/repos/JohnnyEstilles/keystone/milestones{/number}","notifications_url":"https://api.github.com/repos/JohnnyEstilles/keystone/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/JohnnyEstilles/keystone/labels{/name}","releases_url":"https://api.github.com/repos/JohnnyEstilles/keystone/releases{/id}","created_at":"2014-07-07T22:11:18Z","updated_at":"2014-08-02T01:07:17Z","pushed_at":"2014-08-21T02:57:52Z","git_url":"git://github.com/JohnnyEstilles/keystone.git","ssh_url":"git@github.com:JohnnyEstilles/keystone.git","clone_url":"https://github.com/JohnnyEstilles/keystone.git","svn_url":"https://github.com/JohnnyEstilles/keystone","homepage":"http://www.keystonejs.com","size":7124,"stargazers_count":1,"watchers_count":1,"language":"JavaScript","has_issues":true,"has_downloads":true,"has_wiki":true,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":1,"default_branch":"master"}},"base":{"label":"JedWatson:react","ref":"react","sha":"a27e83db86a19292157dfc89b33c507d0f476c68","user":{"login":"JedWatson","id":872310,"avatar_url":"https://avatars.githubusercontent.com/u/872310?v=2","gravatar_id":"6a5996e5c80339e7457d77b7162baacf","url":"https://api.github.com/users/JedWatson","html_url":"https://github.com/JedWatson","followers_url":"https://api.github.com/users/JedWatson/followers","following_url":"https://api.github.com/users/JedWatson/following{/other_user}","gists_url":"https://api.github.com/users/JedWatson/gists{/gist_id}","starred_url":"https://api.github.com/users/JedWatson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/JedWatson/subscriptions","organizations_url":"https://api.github.com/users/JedWatson/orgs","repos_url":"https://api.github.com/users/JedWatson/repos","events_url":"https://api.github.com/users/JedWatson/events{/privacy}","received_events_url":"https://api.github.com/users/JedWatson/received_events","type":"User","site_admin":false},"repo":{"id":11116372,"name":"keystone","full_name":"JedWatson/keystone","owner":{"login":"JedWatson","id":872310,"avatar_url":"https://avatars.githubusercontent.com/u/872310?v=2","gravatar_id":"6a5996e5c80339e7457d77b7162baacf","url":"https://api.github.com/users/JedWatson","html_url":"https://github.com/JedWatson","followers_url":"https://api.github.com/users/JedWatson/followers","following_url":"https://api.github.com/users/JedWatson/following{/other_user}","gists_url":"https://api.github.com/users/JedWatson/gists{/gist_id}","starred_url":"https://api.github.com/users/JedWatson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/JedWatson/subscriptions","organizations_url":"https://api.github.com/users/JedWatson/orgs","repos_url":"https://api.github.com/users/JedWatson/repos","events_url":"https://api.github.com/users/JedWatson/events{/privacy}","received_events_url":"https://api.github.com/users/JedWatson/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/JedWatson/keystone","description":"node.js cms and web app framework","fork":false,"url":"https://api.github.com/repos/JedWatson/keystone","forks_url":"https://api.github.com/repos/JedWatson/keystone/forks","keys_url":"https://api.github.com/repos/JedWatson/keystone/keys{/key_id}","collaborators_url":"https://api.github.com/repos/JedWatson/keystone/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/JedWatson/keystone/teams","hooks_url":"https://api.github.com/repos/JedWatson/keystone/hooks","issue_events_url":"https://api.github.com/repos/JedWatson/keystone/issues/events{/number}","events_url":"https://api.github.com/repos/JedWatson/keystone/events","assignees_url":"https://api.github.com/repos/JedWatson/keystone/assignees{/user}","branches_url":"https://api.github.com/repos/JedWatson/keystone/branches{/branch}","tags_url":"https://api.github.com/repos/JedWatson/keystone/tags","blobs_url":"https://api.github.com/repos/JedWatson/keystone/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/JedWatson/keystone/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/JedWatson/keystone/git/refs{/sha}","trees_url":"https://api.github.com/repos/JedWatson/keystone/git/trees{/sha}","statuses_url":"https://api.github.com/repos/JedWatson/keystone/statuses/{sha}","languages_url":"https://api.github.com/repos/JedWatson/keystone/languages","stargazers_url":"https://api.github.com/repos/JedWatson/keystone/stargazers","contributors_url":"https://api.github.com/repos/JedWatson/keystone/contributors","subscribers_url":"https://api.github.com/repos/JedWatson/keystone/subscribers","subscription_url":"https://api.github.com/repos/JedWatson/keystone/subscription","commits_url":"https://api.github.com/repos/JedWatson/keystone/commits{/sha}","git_commits_url":"https://api.github.com/repos/JedWatson/keystone/git/commits{/sha}","comments_url":"https://api.github.com/repos/JedWatson/keystone/comments{/number}","issue_comment_url":"https://api.github.com/repos/JedWatson/keystone/issues/comments/{number}","contents_url":"https://api.github.com/repos/JedWatson/keystone/contents/{+path}","compare_url":"https://api.github.com/repos/JedWatson/keystone/compare/{base}...{head}","merges_url":"https://api.github.com/repos/JedWatson/keystone/merges","archive_url":"https://api.github.com/repos/JedWatson/keystone/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/JedWatson/keystone/downloads","issues_url":"https://api.github.com/repos/JedWatson/keystone/issues{/number}","pulls_url":"https://api.github.com/repos/JedWatson/keystone/pulls{/number}","milestones_url":"https://api.github.com/repos/JedWatson/keystone/milestones{/number}","notifications_url":"https://api.github.com/repos/JedWatson/keystone/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/JedWatson/keystone/labels{/name}","releases_url":"https://api.github.com/repos/JedWatson/keystone/releases{/id}","created_at":"2013-07-02T05:01:55Z","updated_at":"2014-08-21T01:41:36Z","pushed_at":"2014-08-21T03:34:20Z","git_url":"git://github.com/JedWatson/keystone.git","ssh_url":"git@github.com:JedWatson/keystone.git","clone_url":"https://github.com/JedWatson/keystone.git","svn_url":"https://github.com/JedWatson/keystone","homepage":"http://www.keystonejs.com","size":16405,"stargazers_count":1670,"watchers_count":1670,"language":"JavaScript","has_issues":true,"has_downloads":true,"has_wiki":true,"forks_count":253,"mirror_url":null,"open_issues_count":217,"forks":253,"open_issues":217,"watchers":1670,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/JedWatson/keystone/pulls/559"},"html":{"href":"https://github.com/JedWatson/keystone/pull/559"},"issue":{"href":"https://api.github.com/repos/JedWatson/keystone/issues/559"},"comments":{"href":"https://api.github.com/repos/JedWatson/keystone/issues/559/comments"},"review_comments":{"href":"https://api.github.com/repos/JedWatson/keystone/pulls/559/comments"},"review_comment":{"href":"https://api.github.com/repos/JedWatson/keystone/pulls/comments/{number}"},"commits":{"href":"https://api.github.com/repos/JedWatson/keystone/pulls/559/commits"},"statuses":{"href":"https://api.github.com/repos/JedWatson/keystone/statuses/2b01cf5bf709a2e278aba9f70a6baa702c77e4d0"}}}}