Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Added datetime-local widget, and complete test case. #84

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
2 participants

forms had support for date, but not datetime which is now supported in HTML5. This pull request adds support for inputs.

Collaborator

ljharb commented Nov 5, 2013

This covers datetime-local, but not datetime itself. Should this include both?

I'm also not sure this is worth adding yet - https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Input suggests that neither Firefox nor IE supports it, and that both Chrome and Safari have no special UI for it - ie, there's no benefit to anyone to using it in a browser at this time, except for Opera (which has switched to Blink, and thus no longer supports it).

Hey,

I'll add datetime too. The reason I didn't bother with it, is that datetime isn't supported by chrome, only datetime-local. But I'll add it for completeness.
I think it is still worth adding. It's part of the HTML5 spec and Chrome definitely has a UI for it.

I was going to go through and add support for month, week and time as Chrome has UI for each of those.

It doesn't break any default functionality either, as the standard date widget still exists and can be used without this addition. So I just it as future proofing?

cheers,
Scott.

Collaborator

ljharb commented Mar 12, 2014

Can you rebase this on top of master (and update the tests to use tape)? I'll also leave some additional comments, and then I'll merge this ASAP.

@ljharb ljharb commented on the diff Mar 12, 2014

lib/fields.js
@@ -169,10 +169,18 @@ exports.array = function (opt) {
exports.date = function (opt) {
if (!opt) { opt = {}; }
var f = exports.string(opt);
- if (f.validators) {
- f.validators.unshift(forms.validators.date());
+ if (f.widget && f.widget.type === 'datetime-local') {
@ljharb

ljharb Mar 12, 2014

Collaborator

in this file, rather than the unshift/assign pattern, could you do something with a bit less nesting? Perhaps if (!Array.isArray(f.validators)) { f.validators = []; } and then f.validators.push(…)?

@ljharb ljharb commented on an outdated diff Mar 12, 2014

lib/validators.js
@@ -175,3 +175,16 @@ exports.date = function (message) {
};
};
+exports.datetimelocal = function (message) {
+ if (!message) { message = 'Inputs of type "datetimelocal" must be valid datetimes in the format "yyyy-mm-ddT-hh:mm:ss.000"' }
+ var datetimeRegex = /^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}(:[0-9]{2}\.[0-9]{3})?$/;
@ljharb

ljharb Mar 12, 2014

Collaborator

instead of a regex - could you do something like var date = new Date(message); !isNaN(date.getTime()) && date.toISOString().slice(0, 23) === message?

@ljharb ljharb commented on the diff Mar 12, 2014

lib/widgets.js
@@ -121,6 +121,26 @@ exports.date = function (opt) {
return w;
};
+var dateTimeLocalWidget = input('datetime-local');
+exports.dateTimeLocal = function (opt) {
+ var w = dateTimeLocalWidget(opt);
+ w.formatValue = function (value) {
+
+ if (!value) {
+ return null;
+ }
+
+ var date = value instanceof Date ? value : new Date(value);
@ljharb

ljharb Mar 12, 2014

Collaborator

instanceof isn't a reliable way to detect Dates - forms uses the is module which has an is.date function. can you use that?

Also, new Date(date) should work as well in that case, so you may not even need the check.

@ljharb ljharb commented on an outdated diff Mar 12, 2014

test/test-validators.js
@@ -174,6 +174,29 @@ exports.date = function (test) {
], test.done);
};
+exports.datetimelocal = function (test) {
@ljharb

ljharb Mar 12, 2014

Collaborator

this should maybe be named datetimeLocal, so it reads a bit more cleanly?

I'm going to have to abandon this PR aren't I and start again? You'll note I've had to merge in upstream changes, and there were quite a few which will make this PR a nightmare to merge. BTW, I haven't finished making all of your suggested changes above (but would do in a fresh PR).

Should I start a fresh and bring my code across to a new PR (including adding support for datetime)?

Collaborator

ljharb commented Apr 29, 2014

Whether you abandon it or not is up to you - if you do a rebase, and not a merge, your commit log will be cleaner.

In either case, you'll have to resolve the conflicts unfortunately. I'm fine with either a rebased PR or a new PR, as you like.

Closing this in favour of PR #118.

@smebberson smebberson closed this Apr 30, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment