Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

fix(datepicker): Week number fixes #3158

Closed
wants to merge 2 commits into from
Closed

fix(datepicker): Week number fixes #3158

wants to merge 2 commits into from

Conversation

stephanmullerNL
Copy link

- Reset count at start of year
- Improved algorithm (week number was off by +1) (taken from http://stackoverflow.com/a/7765814/124238)
- Updated tests
@brentmiller
Copy link

I would highly appreciate it if the project collaborators could review this PR. I'm building an application which is impacted by this bug. Thanks!

@mforkin
Copy link

mforkin commented Jan 15, 2015

I haven't look at why yet, but I tried this PR and I am having issues. The boundary between Dec 2010 and January 2011 still shows week 53. I think this is because my calendar starts on sunday but the ISO weeks start on monday.

@Jabe
Copy link

Jabe commented Feb 2, 2015

This PR fixes the calculation, however, the way the UI is implemented right now, it's just not possible to display ISO8601 week numbers if your week doesn't start on Monday.

"It makes sense to start the week on Sunday, the last day of the weekend", said no one ever.

@stephanmullerNL
Copy link
Author

I looked a bit more into ISO 8601, and apparently it is part of the standard that the week must start on a Monday. Hence, supporting ISO 8601 week numbers and having the week start on Sunday are mutually exclusive and prone to errors.

@DumboJet
Copy link

Perhaps I did sth wrong, but after applying the fix to my local JS file manually and setting Monday as the 1st week day, I see that it doesn't work on 2011. On January, it jumps from week #53 to week #2 (which seems odd by all means).

@DumboJet
Copy link

This works great for me:

        Date.prototype.getWeekNumber = function () {
            var d = new Date(+this);
            d.setHours(0, 0, 0);
            d.setDate(d.getDate() + 4 - (d.getDay() || 7));
            return Math.ceil((((d - new Date(d.getFullYear(), 0, 1)) / 8.64e7) + 1) / 7);
        };

Taken from here: http://stackoverflow.com/a/6117889/2173353
I have replaced the original "getISO8601WeekNumber" with it...:

    function getISO8601WeekNumber(date) {
        return date.getWeekNumber();
    }

And I have kept all your other patches.
Cheers! :)

@stephanmullerNL
Copy link
Author

@DumboJet DumboJet, that method looks like it does exactly the same as my current fix. I'm not in a position to check right now but are you sure your code gives different output than mine?

@DumboJet
Copy link

Hm... Probably I had a different version of the lib (I was using ui-bootstrap-tpls-0.11.0.js - I see there is version 12 now...). Here is what I used to have:

  function getISO8601WeekNumber(date) {
    var checkDate = new Date(date);
    checkDate.setDate(checkDate.getDate() + 4 - (checkDate.getDay() || 7)); // Thursday
    var time = checkDate.getTime();
    checkDate.setMonth(0); // Compare with Jan 1
    checkDate.setDate(1);
    return Math.floor(Math.round((time - checkDate) / 86400000) / 7) + 1;
  }

But I've just run this test in my console with your getISO8601WeekNumber():

    new Date('2012/1/1').getWeekNumber()
    52

    (function getISO8601WeekNumber(date) {
            var checkDate = new Date(date);
            checkDate.setDate(checkDate.getDate() + 4 - (checkDate.getDay() || 7)); // Thursday
            var time = checkDate.getTime();
            checkDate.setMonth(0); // Compare with Jan 1
            checkDate.setDate(1);
            return Math.ceil((((time - checkDate) / 86400000) + checkDate.getDay() + 1) / 7);
          })(new Date('2012/1/1'))
    53

Seems to produce slightly different results.
Mine returns 52 and yours 53. I am not sure from where the difference comes from yet or which one is correct.

@DumboJet
Copy link

Well, according to this:
http://www.epochconverter.com/date-and-time/weeknumbers-by-year.php?year=2011
I am correct. :)

@stephanmullerNL
Copy link
Author

It seems I overlooked some details, you are right. I pushed a fix.

However, when using Sunday as the starting day of the week your code is also incorrect. As I mentioned before, I believe this is because the ISO8601 week number and starting the week at sunday are mutually exclusive. I'm still not sure how to approach that problem.

- Improved algorithm yet again
- Updated tests
@DumboJet
Copy link

Yes, I know... Using Sunday as a first day indeed creates problems.
I wish I could help, but I have no idea what should be done for this case either. :(

Actually, I don't even know if it makes sense to apply ISO on that case...
I think it doesn't...

@stephanmullerNL
Copy link
Author

Agreed. I don't think it does either.

@yanivefraim
Copy link

Related: #2306, #3321, #3148

@chrisirhc
Copy link
Contributor

Closing this in favor of #2306 that's a simpler fix. #2306 should also fix all the bugs raised here. If I missed something, reopen.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants