Skip to content

12 hour format, force alt field to always be UTC #492

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

Merged
merged 1 commit into from
Oct 25, 2012

Conversation

rocksolidwebdesign
Copy link
Contributor

Here is one fix and one feature

12 hour formatting option

The fix addresses issue(s) #459 and #479

to use the fix simply use a capital "H" instead of a lower case "h" in your time format specifier to force usage of 12 hour time format, "HH" with leading zero and "H" without, for example. The ampm option now only affects the process of time picking and not the resulting output. This will, for example, display times in 12 hour format, but the time picker should remain in 24 hour format

$("foo").datetimepicker({
      "timeFormat": "H:mm",
      "altTimeFormat": "HH:mm:ss"
});

not that you'd want this ever, but the feature works the other way as well - so of course, you can still use lower case "h" to get a nice display format on the frontend and a more easily parsable format for the backend

$("foo").datetimepicker({
      "timeFormat": "H:mm",
      "altTimeFormat": "hh:mm:ss"
});

working demo: http://jsfiddle.net/SyNCp/1/

@trentrichardson
Copy link
Owner

Hey Vaughn, Thanks for your work here. A couple notes.

  • Yesterday I added to the dev branch altAmpm option, which may fix issue converting from ampm back to 24 hour time does not work. #479.
  • I don't have any problem with using H and h, but if we go that route I would try to eliminate the need for ampm and h/H. I probably should have gone this route from the beginning. Having to specify both could get confusing.
  • All localizations need to be updated if ampm option is dropped.
  • You mention ampm is only used for parsing with your change. I have been brainstorming on how to redo parsing. Currently it is strict, and by that I mean the time input must match the timeFormat exactly. I have been brainstorming a couple ways to parse the time. This may not directly affect your changes but I think it the parsing will have to be reworked to eliminate the ampm option.

Parsing Time Option 1:
Loop through a list of predetermined formats, the first format of course being our timeFormat. Once it finds a match it uses it

Parsing Time Option 2:
Use javascript "new Date(dateTimeString)" to try and parse the datetime. It can recognize many formats by itself. If that fails use our current strict method.

Do you have any thoughts or suggestions? I believe leaning more on timeFormat than a separate ampm option is the way to go, I just want to make sure all ends (parsing and outputing) are on the same page before pushing a live version.

@rocksolidwebdesign
Copy link
Contributor Author

Hey Trent,

actually, ampm is NOT used for parsing, it's only used for controlling the display of the sliders, which IMO is and should be a separate setting from the control of the regular and alt inputs. the ampm setting no longer affects the internal time values, that is, it never affects/changes the "hour" variable, rather that's left to a 12-24hr conversion function when invoked by using the new HH format specifier

I think the current situation is actually good because now there are three different discreet and decoupled controls for 12 vs 24 hour format

  • ampm controls display on the slider
  • timeFormat controls this.$input, and
  • altTimeFormat controls this.$altInput

and they can all be independent and different if desired which I have found can be a problem - this is in fact currently a problem in another "competing" plugin - data format should, in the ideal case, control/force/dictate display format as little as possible and I consider the formats of the input boxes to be much more "data" oriented than the format of the slider

I guess it might be fair to say that one would never set timeFormat to use the 12 hour format and then have the sliders display 24 hour format, however I have run in to the the reverse problem already, and even though it may be an unlikely instance, I think the benefits of decoupling are worth leaving this as is

If you wish to refactor because you have ampm coupled with some parsing, I would encourage you to move to a slightly different parameter that's meant to be interface-display only, aka only controlling the display of the sliders, and probably call it "use12HourInterface" or some such.

@rocksolidwebdesign
Copy link
Contributor Author

and by the way, I think I do understand what you mean in regards to parsing - for now I am getting dates from Rails and they come in the ISO 0000-00-00T00:00:00Z format which is easily/automagically acceptable by JavaScript's Date object constructor and I personally have been using .datepicker("setDate", someDateObject); so I don't really have to worry about parsing from an input value="" attribute

if you want to be able to pull in a string from the value="" attribute of the input box to populate the Timepicker instance with time values, then yeah I think parsing will need to be worked on here

@trentrichardson
Copy link
Owner

On the topic of ampm option, how about this alternative, would be much more consistant (Just bouncing ideas around):

A "pickerTimeFormat" option, similar to timeFormat and altTimeFormat, this one would be used for display inside the picker and depending on it containing H or h it would handle the sliders formatting. It could default to timeFormat, and this way users can adjust how they like inside the picker?

@rocksolidwebdesign
Copy link
Contributor Author

yeah I think that's a fine idea - I think it would also go well towards implementing another feature I personally see as semi critical which is that I think it would be ideal to allow the datetimepicker widget to hide all normal/related text-input fields and permanently display itself directly in the DOM, this as opposed to being hidden until the popup is triggered by a click

the anytime picker lets you do this and I think it's a good feature and would be of high value to implement

having a pickerTimeFormat option would work well with a feature like that

@rocksolidwebdesign
Copy link
Contributor Author

some more thoughts as per your original comment about parsing options 1 and 2

I think it's safe/sane to say that the timeFormat to use when parsing a given <input value=""> value should be the format which belongs to that field, if it's the primary field then it should be dateFormat and timeFormat, and if it's the alt field it should be altDateFormat and altTimeFormat with fallbacks to defaults for each

as for whether to try parsing naturally with JS first, we could have a flag such as "tryToPlayNice" maybe - when set to true, it will try to just parse the date string via new Date() with a fallback to the format options and then a final fallback to the defaults, and if set to false it'll always parse using the time and date format and won't attempt to use new Date()

alternatively we may even wish to add a few more discreet options, "parseDateFormat", "parseTimeFormat", "altParseDateFormat" etc. respectively which would mirror dateFormat and timeFormat

this would possibly also require an option such as altParseSeparator

@trentrichardson
Copy link
Owner

Ok, So my gameplan looks like:

Input

  • add a "strictParse" or similar option which true uses timeFormat exactly, false tries new Date, then if fail some common formats (first format being tried is given timeFormat)

Output

  • bring in your changes
  • add pickerTimeFormat, defaults to timeFormat to format the time label inside the picker
  • drop ampm option, replace with detecting lowercase h in the given format

trentrichardson added a commit that referenced this pull request Oct 25, 2012
12 hour format, force alt field to always be UTC
@trentrichardson trentrichardson merged commit 19dfb8c into trentrichardson:dev Oct 25, 2012
@trentrichardson
Copy link
Owner

I merged your changes to the dev branch. I looked at a few other language's timeFormat (php, python, coldfusion, etc..) and H commonly refers to 24 hour clock, and h refers to 12 hour clock, so I swapped those out. Added pickerTimeFormat and dropped ampm options. I added an internal method useAmpm(timeFormat) to detect when to use ampm from the timeFormat (it looks for h and t to be present).

There is also a "strict" option which defaults to true for parsing. When false it will try new Date, if it fails it will try strict, else throws an error.

@rocksolidwebdesign
Copy link
Contributor Author

cool, sounds good - I look forward to the rest of it

thanks

@Gunky
Copy link

Gunky commented May 22, 2013

I've searched quite some time to fix this issue for ampm display settings. On a webapp I'm working on, the users can switch for 24h display or 12h ampm.

So the ampm option is gone .. ?

I fixed it, thanks to this post, to the following code:
timeFormat: (userPreferences.AMPM ? 'hh:mm tt' : 'HH:mm')

Only thing what is a bit pity, is the documentation of the datetimepicker. I love all of your work and can only encourage it. If you need help for documenting all of this, just pm ;)

@trentrichardson
Copy link
Owner

Hey Gunky,

The ampm option is gone and pulls from the timeFormat like you have there. Now all support pulls from each timeFormat, pickerTimeFormat, altTimeFormat.

The docs need to be redone sometime soon, but for the time being is there a particular item missing that needs to be added?

@Gunky
Copy link

Gunky commented May 23, 2013

Hi Trent,

Well, like the ampm option which was deleted, I never found anything of it, except in this post. Would be handy if developers know how they can implement ampm setting :)

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.

3 participants