Opened 7 years ago

Closed 7 years ago

#15081 closed enhancement (fixed)

mobile: use Date objects as value for date picker widgets

Reported by: ben hockey Owned by: ykami
Priority: high Milestone: 1.8
Component: DojoX Mobile Version: 1.7.2
Keywords: Cc: Eric Durocher
Blocked By: Blocking:

Description

related to #15063.

improve the mobile Date pickers to work with a Date as a value

picker.set('value', new Date());

picker.get('value'); // returns a Date

Attachments (4)

_DatePickerMixin.patch (1.6 KB) - added by Eric Durocher 7 years ago.
Porposed implementation of picker.set('value', '<ISO-date-string>')/picker.get('value')
test_ValuePickerDatePicker-iso.html (3.6 KB) - added by Eric Durocher 7 years ago.
test case for previous patch
_DataPickerMixin.patch (1.6 KB) - added by Eric Durocher 7 years ago.
Proposed implementation of picker.set('value', '<ISO-date-string>')/picker.get('value')
15081.patch (7.5 KB) - added by Eric Durocher 7 years ago.
New patch, ignore previous patches - Eric Durocher (IBM, CCLA)

Download all attachments as: .zip

Change History (30)

comment:1 Changed 7 years ago by Adam Peller

Javascript Date objects are just horrible. Perhaps it would be good to use the ISO string representation instead? I think that's what bill is thinking for Dijit 2.0

comment:2 Changed 7 years ago by ben hockey

i'll settle for anything that is the value property and (de)serializes easier than the array [2010, 9, 1]. ISO strings would be ok. consistency with dijit is the real goal here.

comment:3 Changed 7 years ago by Adam Peller

ISO is nice for several reasons. It's trivial to parse (split or regexp), you can go back and forth to Date objects as needed with dojo/date/stamp or native ES5 methods, you don't have to represent months as n-1, and you can leave times and timezones out of the picture entirely. They're also much cheaper than Date objects.

You're dealing with only the date, but you'd probably want to tolerate date-times as input: yyyy-MM-ddTHH:mm:ss... I suppose that raises the issue of which timezone to use.

comment:4 Changed 7 years ago by ykami

Sounds like a nice to have feature. Do you think it's worth adding extra code for this?

comment:5 Changed 7 years ago by ben hockey

i believe it would be common enough that if the code isn't added to the widget it will likely exist in some form in almost every application that uses this widget. if it is left to developers to add code for serializing/deserializing values for this widget themselves it will also likely be code that gets repeated within the application.

i think adam makes a good point about ISO strings and that format would be ideal for transferring data across the wire so serializing/deserializing the value as an ISO string would be a good option.

how much extra code would it be?

comment:6 Changed 7 years ago by Adam Peller

The ISO code drag shouldn't be too bad. dojo/date/stamp is small, and pretty close to something we can has-optimize out with ES5, or if we can assume ES5 going forward, there's no code drag at all.

Using ISO dates for external APIs is one thing, but if we want to use some sort of simple string representation internally also, I guess you could just steal the parsing pieces and never turn it into a date... a split or a regex, pretty simple if you don't have to worry about defaults. Of course, a big benefit is that the representation would then be date-only and not date-time with timezone issues.

Question then is, how do we deal with multiple calendar support? We've been using the Javascript Date object as an abstraction. We could use ISO dates for external APIs, but we'd need some way to abstract it in our implementation.

comment:7 Changed 7 years ago by ykami

OK, that makes sense. SpinWheel is relatively a little bigger widget (taking its dependent modules into account). Adding dojo/date/stamp does not seem to have too much impact.

comment:8 Changed 7 years ago by ykami

Cc: Eric Durocher added

comment:9 Changed 7 years ago by ykami

Milestone: tbd1.8
Priority: undecidedlow

Changed 7 years ago by Eric Durocher

Attachment: _DatePickerMixin.patch added

Porposed implementation of picker.set('value', '<ISO-date-string>')/picker.get('value')

comment:10 Changed 7 years ago by Eric Durocher

Here is a patch that adds a 'value' attribute to the _DatePickerMixin, so you can do:

picker.set('value', '<iso-date-string>');
var iso_date_string = picker.get('value');

Changed 7 years ago by Eric Durocher

test case for previous patch

comment:11 Changed 7 years ago by Adam Peller

I'm unable to find the code this patches. If the public API is still using triples (array) elsewhere, I suggest using ISO dates *instead*. This would provide a more consistent API with the rest of Dojo. Also, the n-1 month index is always a source of confusion.

Last edited 7 years ago by Adam Peller (previous) (diff)

Changed 7 years ago by Eric Durocher

Attachment: _DataPickerMixin.patch added

Proposed implementation of picker.set('value', '<ISO-date-string>')/picker.get('value')

comment:12 Changed 7 years ago by Eric Durocher

The code is dojox/mobile/_DatePickerMixin.js - attached a new patch created from the root, sorry.

comment:13 Changed 7 years ago by ykami

neonstalwart, peller, does the patch look good to you?

comment:14 Changed 7 years ago by Adam Peller

sure, I guess the only thing I'd question is failing silently on bad input. perhaps just let it throw?

comment:15 Changed 7 years ago by ben hockey

can you add some assertions/tests to the test? i want to make sure this doesn't regress.

also, this whole DatePicker? code looks kind of strange

  • when someone requires DatePicker?, they asynchronously get the prototype changed! this makes programmatic instantiation of DatePicker? basically impossible. see #15062
  • you've got properties like slotClasses and slotProps that are "static" properties to be shared across all instances! this means that changing slotProps for one instance could possibly affect different instances.
  • reorderSlots is using a private method of datelocale var a = datelocale._parseInfo().bundle["dateFormat-short"].toLowerCase().split(/[^ymd]+/, 3);
  • when you set the value, you already have the string in a known format, yet it gets converted to a date and then back to a string again. isn't there a way to use the string without needing to convert it to a date and back again?
  • the values property (which is expected to be an array) probably shouldn't be part of the public API since an array of strings is not a typical format for date values.

that's all i have time for right now.

comment:16 Changed 7 years ago by ykami

when someone requires DatePicker??, they asynchronously get the prototype changed! this makes programmatic instantiation of DatePicker?? basically impossible. see #15062

Right, it doesn't work as you expected since it requires either of the modules asynchronously. My intention for DatePicker/TimePicker was basically for declarative use cases, because you can easily choose whichever widget programmatically, but I thought it would not be easy to do so declaratively. Also, I believe this style of programmatic instantiation should work:

require([
  ...
  "dojox/mobile/DatePicker"
], function(ready, DatePicker){
  ready(function(){
    var w = new DatePicker({}, "picker1");
    w.startup();
    ...
  });
});

That said, it would be better if your way of instantiation worked as well. Do you have any idea to achieve that?

you've got properties like slotClasses and slotProps that are "static" properties to be shared across all instances! this means that changing slotProps for one instance could possibly affect different instances.

Oops, you are correct, sorry I should have noticed earlier. (The original code came from my colleague..)

reorderSlots is using a private method of datelocale var a = datelocale._parseInfo().bundledateFormat-short?.toLowerCase().split(/[ymd]+/, 3);

Right. But I couldn't find a better way to do it.

when you set the value, you already have the string in a known format, yet it gets converted to a date and then back to a string again. isn't there a way to use the string without needing to convert it to a date and back again?

For English locale, you may be right. But other locales may need conversion.

the values property (which is expected to be an array) probably shouldn't be part of the public API since an array of strings is not a typical format for date values.

It comes from the base class, whose usage is not limited to date picker, and it is used internally. You may be correct, the string array setter should be used only internally. Maybe we should document the values setter for date picker takes an integer array.

comment:17 Changed 7 years ago by Eric Durocher

New patch uploaded (sorry for the mixup in patch names... :-} ):

  • set('value') now throws an error on invalid input
  • changed the test into a DOH unit test
Last edited 7 years ago by Eric Durocher (previous) (diff)

comment:18 Changed 7 years ago by Adam Peller

the dojo way is usually not to check the arguments, but to just let it fail (and be sure that it does) to save the extra bytes of code. There's certainly a good argument to be made for explicitly checking and throwing comprehensible error messages. I suppose such style would be up to the module owner.

Last edited 7 years ago by Adam Peller (previous) (diff)

Changed 7 years ago by Eric Durocher

Attachment: 15081.patch added

New patch, ignore previous patches - Eric Durocher (IBM, CCLA)

comment:19 Changed 7 years ago by Eric Durocher

Yes I was wondering about that actually... OK, a byte is a byte especially on mobile, I removed the explicit throw (and added the error case in the unit test). Thanks!

comment:20 Changed 7 years ago by ykami

Priority: lowhigh
Status: newassigned

(I didn't mean to mark this ticket as low priority)

comment:21 Changed 7 years ago by ykami

In [28297]:

Refs #15081 !strict. Added value setter and getter that handle ISO dates to _DatePickerMixin. Also added a test case for that. Thank you edurocher for the patch.

comment:22 Changed 7 years ago by ykami

As for the "static" properties like slotClasses, I checked the code, but couldn't find any problems with that, because they are referenced only in buildRendering, that is, changing those values at runtime does not make sense. Do you have any specific scenarios that may have the problem?

comment:23 Changed 7 years ago by ykami

In [28326]:

Refs #15081 It turned out that [28297] works only in locales that have month names (e.g. English). For other locales (e.g. Japanese), month is a number, and it is converted again in _setValuesAttr, and that results in n+1 month. Fixed _setValuesAttr so that users can specify month that starts from 1, not zero. !strict

comment:24 in reply to:  22 Changed 7 years ago by ben hockey

Replying to ykami:

As for the "static" properties like slotClasses, I checked the code, but couldn't find any problems with that, because they are referenced only in buildRendering, that is, changing those values at runtime does not make sense. Do you have any specific scenarios that may have the problem?

i haven't had a chance to see what happens when i use this. i'm just very suspicious of "static" properties. if you've looked at it and you say its ok then i'll accept that.

comment:25 Changed 7 years ago by ykami

ok, thanks. You are correct, they may be a bit confusing. Their scope should be something like 'protected' in Java, since they are intended to be used from subclasses like SpinWheelDatePicker. They are not necessary for typical programmatic usage. Maybe I should make them private to avoid confusion.

comment:26 Changed 7 years ago by ykami

Resolution: fixed
Status: assignedclosed

I think we're done.

Note: See TracTickets for help on using tickets.