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

Allow typing in date widget #1247

Merged
merged 5 commits into from
Apr 14, 2018

Conversation

Dammmien
Copy link
Contributor

Fixes #1178 : Allow typing in date widget

- Summary

Allow typing in date widget by setting the date only if the format is valid

- Test plan

Test date widget behavior is ok

- Description for the changelog

Allow typing in date widget

@verythorough
Copy link
Contributor

verythorough commented Apr 11, 2018

Deploy preview for cms-demo ready!

Built with commit 532b311

https://deploy-preview-1247--cms-demo.netlify.com

@verythorough
Copy link
Contributor

verythorough commented Apr 11, 2018

Deploy preview for netlify-cms-www ready!

Built with commit 532b311

https://deploy-preview-1247--netlify-cms-www.netlify.com

@@ -36,9 +36,8 @@ export default class DateControl extends React.Component {

handleChange = datetime => {
const { onChange } = this.props;
if (!this.format || datetime === '') {
Copy link
Contributor

Choose a reason for hiding this comment

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

We do still need to handle the empty case here, in case the user wants to empty the field.

@tech4him1
Copy link
Contributor

This looks great -- I think reverting to the previous value if the date is invalid is reasonable. Do you think we need to warn the user at that point, or just revert it? For simple changes, the user wouldn't necessarily notice that it was reverted (e.g. entered SM instead of AM).

@Dammmien
Copy link
Contributor Author

@tech4him1 The empty case is now supported and the user is warned onBlur that the date is invalid.

@erquhart
Copy link
Contributor

This is definitely an improvement, thanks @Dammmien!

I do find the warning jarring, especially considering the case where a user types in a date, and it's almost guaranteed to be the wrong format, then they get a generic warning and the field goes blank.

I'd like us to consider attempting to moment.parse whatever they type, and use the result if it's valid. Moment is really good at parsing dates, and while much of the fast and loose parsing is deprecated, their 3.0 milestone is years old and looks unlikely to ever come to fruition. In the future we could also move to date-fns which seems quite happy to parse strings as well.

Thoughts?

@tech4him1
Copy link
Contributor

Also, if we do choose to use moment.parse, I don't see any problem with just using the functions in react-datetime directly to parse and check validity. We should simply be able to set the dateFormat on it.

@Dammmien
Copy link
Contributor Author

@erquhart , @tech4him1
I pushed a new version, where the input is parsed with moment(...) if the date is valid it's used otherwise it's reverted to the previous value.
What do you think about it ?

Copy link
Contributor

@erquhart erquhart left a comment

Choose a reason for hiding this comment

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

This is awesome, works really well. Thanks @Dammmien!

Copy link
Contributor

@tech4him1 tech4him1 left a comment

Choose a reason for hiding this comment

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

I think this looks great, just one quick change.

@@ -34,16 +34,35 @@ export default class DateControl extends React.Component {
}
}

// Date is valid if datetime is a moment or Date object otherwise it's a string.
// Handle the empty case, if the user wants to empty the field.
isValid = datetime => (moment.isMoment(datetime) || datetime instanceof Date || datetime === '');
Copy link
Contributor

Choose a reason for hiding this comment

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

isValid has a specific use for widget controls, which we're not trying to use here, so can we change this to something else, maybe isValidDate?

@tech4him1
Copy link
Contributor

@erquhart Just so I understand -- we're not warning users if the date is invalid because of why? Now that we are parsing with Moment it should rarely be invalid.

@erquhart
Copy link
Contributor

erquhart commented Apr 13, 2018

Right, so you'd have to type something pretty odd to get a blank field. The warning was a tad jarring anyhow, I don't think anyone will be thrown by this behavior. What do you think?

@tech4him1
Copy link
Contributor

tech4him1 commented Apr 13, 2018

Is there any reason we can't throw the warning only if the value can't be parsed by Moment, since that should be rare now?

@erquhart
Copy link
Contributor

It is clearer that way - your call, I'm good with either.

Sent with GitHawk

@Dammmien
Copy link
Contributor Author

@tech4him1 I changed isValid to isValidDate and restored the warning when the value can't be parsed by Moment.

@tech4him1 tech4him1 merged commit 9975c7e into decaporg:master Apr 14, 2018
@Dammmien Dammmien deleted the 1178_widget_date_typing branch April 26, 2018 20:15
brianlmacdonald pushed a commit to brianlmacdonald/netlify-cms that referenced this pull request May 23, 2018
* Allow typing in date widget

* Handle empty case & warn user when invalid

* Try parsing date with moment

* Rename isValid -> isValidDate

* Warn user when date is invalid
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