icambron / twix.js

:hourglass::left_right_arrow: A date range plugin for moment.js
https://isaaccambron.com/twix.js/
MIT License
379 stars 54 forks source link

Add 'implicitMonth' option #88

Closed wpscholar closed 8 years ago

wpscholar commented 8 years ago

Currently, I am getting smart formatted ranges like this: September 14 - September 21, 2016

It would be great to add an implicitMonth option so that you would get the expected (and much shorter) formatting: September 14 - 21, 2016

icambron commented 8 years ago

This is kind of a bug, actually. I assume you're using hideTime? If so, here's what's happening:

//Twix totally knows to collapse months for all-day ranges:
> moment().twix(moment().add(4, 'd'), {allDay: true}).format()
'Jul 2 - 6'

//But it doesn't do it for regular ranges because it would look weird:
> moment().twix(moment().add(4, 'd').add(3, 'h')).format()
'Jul 2, 2:13 AM - Jul 6, 5:13 AM'

//Imagine Jul 2, 2:13 AM - 6, 5:13 AM, which is horrible

//However, if you use regular ranges and hide the times, we should collapse it:
> moment().twix(moment().add(4, 'd').add(3, 'h')).format({hideTime: true})
'Jul 2 - Jul 6'

//but we don't because the should-I-collapse-this logic doesn't consider
//whether you're hiding the time, just whether the actual range is all-day or not

I'm going to fix this shortly by just adding that consideration rather than an option. You can fix it in the super short term by using an all-day range instead (I don't know your exact use case, but you might consider using one regardless if the dates are really the only thing that matters to the definition of these ranges -- they do lots of other helpful stuff too!)

icambron commented 8 years ago

Fixed in 5c6baa99cf56e58f72b562ac074ee10477d652e7, released in Twix 1.1.0. Thanks for the bug report!

(and if I was wrong about your use of the hideTime option, let me know and I'll reopen)

wpscholar commented 8 years ago

Thanks for the feedback. Here is my original code:

var range = startDate.twix(endDate);
var twixSettings = {
    implicitYear: false,
    monthFormat: 'MMMM',
    meridiemFormat: 'a'
};

if ('12:00AM' === startDate.format('h:mmA') && '12:00AM' === endDate.format('h:mmA')) {
    twixSettings.hideTime = true;
}
humanReadableEventDate = range.format(twixSettings);

So you were correct on my use of hideTime. I tried using explicitAllDay: true and allDay: true as formatting options to no avail. I did get it to work based on your example, but it seems a bit hacky in my use case because I couldn't use allDay: true in the twixSettings when formatting... I could only use it as the second param when creating the range. Which means the new code looks like this:

var range = startDate.twix(endDate);
var twixSettings = {
    implicitYear: false,
    monthFormat: 'MMMM',
    meridiemFormat: 'a'
};

if ('12:00AM' === startDate.format('h:mmA') && '12:00AM' === endDate.format('h:mmA')) {
    range = startDate.twix(endDate, {allDay: true});
}
humanReadableEventDate = range.format(twixSettings);

End result is I had to generate a completely new range to get the desired effect.

SIDE NOTE: The confusing thing was looking at the documentation and seeing that the formatting option for the allDay argument defaults to all day. So obviously this appears to be a different argument than what I passed in to my new code when creating the range. However, since the default value isn't a boolean, I can't imply what the other options are and the documentation doesn't provide any detail on other possible values.

wpscholar commented 8 years ago

PS... I didn't have time to update, but did see that you've provided a fix. I just wanted to point out a few other things I encountered in case it is helpful. Thanks!

icambron commented 8 years ago

The confusing thing was looking at the documentation and seeing that the formatting option for the allDay argument defaults to all day.

Yeah, that could definitely be clearer. The allDay argument to format() means something different than the allDay argument to twix(). It controls this:

> moment().twix(moment(), {allDay: true}).format({implicitDate: true})
'all day'
> moment().twix(moment(), {allDay: true}).format({implicitDate: true, allDay: 'the whole day'})
'the whole day'
> moment().twix(moment(), {allDay: true}).format({explicitAllDay: true, allDay: 'the whole day'})
'the whole day Jul 2'

That weird naming aside, I think creating a whole new Twix with modified options actually makes the most sense, but there needs to be a good function for it, like range.clone({allDay: true}).

But hopefully, my fix addresses your issue regardless!