Skip to content
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

Fixed bug with getWeekId #112

Closed
wants to merge 1 commit into from
Closed

Fixed bug with getWeekId #112

wants to merge 1 commit into from

Conversation

AWilco
Copy link

@AWilco AWilco commented Dec 21, 2013

Theres an apparent bug in getWeekId (Dec 31, 2013 would give 'dh-2013-0' instead of...'dh-2014-0'). This appears to be because getDayForWeek might move to the next year (week 0 may include some dates from the previous year). The if (m === 11 && w === 1) { appeared to be a workaround, but doesnt work because week numbers start at 0. However just basing the id of getDayForWeek() seems to fix it and be simpler.

@usmonster
Copy link
Collaborator

I haven't tested, but it looks like a reasonable patch. I'd rather replace all the methods that modify the Date prototype with regular stand-alone functions, though (eventually). @taitems, any opinion? We could merge this as an intermediate patch, which could also fix some other outstanding bugs (e.g., #61, #99, ...).

@usmonster usmonster added the bug label Apr 23, 2014
diogenesl added a commit to diogenesl/jQuery.Gantt that referenced this pull request Sep 15, 2014
Fixes bugs:
Weekly scale showing week zero in some situations (taitems#112)
Weekly scale not showing correct number of weeks (52 or 53) (taitems#99)
Daily scale showing day 1 inside the previous month group

PS: This is my first GitHub contribuition, hope that I'm doing everything right.
@usmonster usmonster added this to the v1.3.0 milestone Dec 21, 2014
@usmonster usmonster closed this in a745147 Sep 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants