Opened 9 years ago
Closed 9 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)
Change History (30)
comment:1 Changed 9 years ago by
comment:2 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
Sounds like a nice to have feature. Do you think it's worth adding extra code for this?
comment:5 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
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 9 years ago by
Cc: | Eric Durocher added |
---|
comment:9 Changed 9 years ago by
Milestone: | tbd → 1.8 |
---|---|
Priority: | undecided → low |
Changed 9 years ago by
Attachment: | _DatePickerMixin.patch added |
---|
Porposed implementation of picker.set('value', '<ISO-date-string>')/picker.get('value')
comment:10 Changed 9 years ago by
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 9 years ago by
Attachment: | test_ValuePickerDatePicker-iso.html added |
---|
test case for previous patch
comment:11 Changed 9 years ago by
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.
Changed 9 years ago by
Attachment: | _DataPickerMixin.patch added |
---|
Proposed implementation of picker.set('value', '<ISO-date-string>')/picker.get('value')
comment:12 Changed 9 years ago by
The code is dojox/mobile/_DatePickerMixin.js - attached a new patch created from the root, sorry.
comment:14 Changed 9 years ago by
sure, I guess the only thing I'd question is failing silently on bad input. perhaps just let it throw?
comment:15 Changed 9 years ago by
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
andslotProps
that are "static" properties to be shared across all instances! this means that changingslotProps
for one instance could possibly affect different instances. reorderSlots
is using a private method of datelocalevar 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 9 years ago by
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 9 years ago by
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
comment:18 Changed 9 years ago by
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.
Changed 9 years ago by
Attachment: | 15081.patch added |
---|
New patch, ignore previous patches - Eric Durocher (IBM, CCLA)
comment:19 Changed 9 years ago by
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 9 years ago by
Priority: | low → high |
---|---|
Status: | new → assigned |
(I didn't mean to mark this ticket as low priority)
comment:22 follow-up: 24 Changed 9 years ago by
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:24 Changed 9 years ago by
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 9 years ago by
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.
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