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

Adding date type react component #559

Merged
merged 9 commits into from
Aug 21, 2014

Conversation

estilles
Copy link

This date component is functionally equivalent to its "pre-react" counterpart.

I 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.

On 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.

I'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.

  1. 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.
  2. 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.)

Also, I noticed we're getting a moment() deprecation warning on the server-side updateItem() method for the date field.

Deprecation warning: moment construction falls back to js Date. This is discouraged and will be removed in upcoming major release. Please refer to moment/moment#1407 for more info.

Moment now requires the format argument when parsing a date.I will clean that up on my next PR.

Let me know what you all think about my suggested tweaks and let's talk about client-side validation (I think it's long overdue).

@estilles estilles changed the title Adding date type component Adding date type react component Aug 21, 2014
@@ -17,6 +17,8 @@ function date(list, path, options) {
this._underscoreMethods = ['format', 'moment', 'parse'];

this._formatString = (options.format === false) ? false : (options.format || 'Do MMM YYYY');
this._properties = ['formatString'];
this.formatString = this._formatString;
Copy link
Member

Choose a reason for hiding this comment

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

I'm going to think on a cleaner way to support this, rather than doubling up the property. Or remove the underscore in the first place.

Copy link
Author

Choose a reason for hiding this comment

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

I vote for removing the ._formatString.

@JedWatson
Copy link
Member

Thanks, @JohnnyEstilles.

re: 1. "Use strict parsing with moment()" - agreed, let's do this as a separate PR though

  1. "Add an optional feature that will prevent user input and force selection from date picker, and another that ONLY shows the date picker." - sounds like a good idea, also as a separate PR.

I'm really interested in implementing client-side validation, I think it would be great if it could share the same code as the server-side too. Agreed it's long overdue but maybe we should get through more of the React update first? A lot of things are going to move around...

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.

2 participants