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

fix(datepicker): week count issues #2306

Closed
wants to merge 5 commits into from
Closed

Conversation

a5sk4s
Copy link
Contributor

@a5sk4s a5sk4s commented Jun 8, 2014

This fixes a few issues with week counting

(1) Week counts in January are counted as last years' weeks when the first Thursday is from last year (e.g., 53, 54, 55, ...). See January 2010, for example.

(2) Week counts in December continue counting the last week(s), which may be part of next year, as weeks from the current year. See December 2014, for example.

(3) Week counting changes depending on the start day. The week containing June 7th 2014 is week 22 with the default start day (Sunday), but week 23 with start day Monday. See examples on http://angular-ui.github.io/bootstrap/ (popup has start day Monday)

The code has been changed so that the Thursday of the week always determines, which week count the week gets assigned, regardless of the start day of the week.

@a5sk4s
Copy link
Contributor Author

a5sk4s commented Jun 8, 2014

I also changed the test it('renders the week numbers based on ISO 8601', ...), which apparently was based on the implementation and not on the real values when comparing it to other implementations.

@karianna karianna added this to the 0.13.0 milestone Mar 6, 2015
@yanivefraim
Copy link

Related: #3321, #3158, #3148

@a5sk4s
Copy link
Contributor Author

a5sk4s commented Mar 8, 2015

I believe it is still relevant - do not have a lot of time at the moment to dive into it again - just fixed a left ddescribe in the spec file, though and merged master into the branch - happy to help, if there are any questions, though.

@rvanbaalen
Copy link
Contributor

Thanks for double checking! We'll be pulling in PRs soon and this one looks pretty good. Hold on tight!


This email was sent from my iPhone and therefore subject to typos and other inaccuracies.

On 8 mrt. 2015, at 01:14, a5sk4s [email protected] wrote:

I believe it is still relevant - do not have a lot of time at the moment to dive into it again - just fixed a left ddescribe in the spec file, though and merged master into the branch - happy to help, if there are any questions, though.


Reply to this email directly or view it on GitHub.

@chrisirhc
Copy link
Contributor

Removed my previous wrong comment as I understand the problem better now.

});

describe('first week in january', function() {
beforeEach(inject(function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This empty beforeEach can be removed

@chrisirhc
Copy link
Contributor

Good work here. Left some comments above. Needs rebasing though.

@stephanmullerNL
Copy link

Great stuff, thanks!

@wesleycho
Copy link
Contributor

I think we should try to get this in if there are no other issues other than stylistic issues - we can do the refactoring necessary when merging this in. The history needs to be fixed though.

@chrisirhc
Copy link
Contributor

Merged. Thank you!

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.

7 participants