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

[Bug] Time-Range rendered incorrectly (because of buggy calendar week calculation) #61

Open
FunkMonkey opened this issue Jan 8, 2013 · 10 comments

Comments

@FunkMonkey
Copy link

When zoomed to the week scale, the timerange from 01.01.2012 to 31.12.2013 is rendered incorrectly. It will just have the width of the containing text.

Problem is the wrong calculation of calender weeks (f.ex 2014 has a 0 calendar week). also it seems there are different ways the calendar week is calculated in the code instead of just one.

Here is an example gantt:

$(".gantt").gantt({
                scale: "weeks",
                scrollToToday: false,
                source: [{
                    name: "Sprint 0",
                    desc: "Analysis",
                    values: [{
                        from: "/Date(1325372400000)/",
                        to: "/Date(1388444400000)/",
                        label: "Requirement Gathering", 
                        customClass: "ganttRed"
                    }]
                }] });

(jsFiddle)

I vote for using the calendar week definition of ISO 8601

@qflamol
Copy link

qflamol commented Feb 4, 2013

Hello FunkMonkey,

Have you found a solution for that problem? I am experience the same issue and I am trying to find a way to fix it since my calendar is not showing the right data.

Thank you,
Flavius.

@qflamol
Copy link

qflamol commented Feb 10, 2013

Based on FunkMonkey's comment and jquery.ui, I replaced the original function from "jquery.fn.gantt.js" with the following one and it seems to fix the problem:

Date.prototype.getWeekOfYear = function () {
    var ys = new Date(this.getFullYear(), 0, 1);
    var sd = new Date(this.getTime());

    // Find Thursday of this week starting on Monday
    sd.setDate(sd.getDate() + 4 - (sd.getDay() || 7));

    return Math.floor(Math.round((sd - ys) / 86400000) / 7) + 1;
};

@FunkMonkey
Copy link
Author

Unfortunately it is not that easy.

Test your function for the 31.12.2013. It will say that it is calendar week 53, though it should actually be calendar week 1 of 2014.

Thus, for making a correct calculation you may actually need the number of calender weeks of the last year (in case the first days of the new year belong to the last calendar week of the previous year) or the start of the first calendar week in the next year (in case a year's last days belong to the first week of the new year).

You can test your code with the calendar weeks from this site: kalenderwoche.net. It is in german, but there is not much to read anyway...

p.s. the initial problem for the bug though was the different calendar-week calculation of the columns vs the gantt-lines

@taitems
Copy link
Owner

taitems commented Feb 10, 2013

Can I also recommend you guys have a squizz the dev branch to verify fixes, and write your own tests too?

@qflamol
Copy link

qflamol commented Feb 13, 2013

Hmmm, you are right. Can you please take a look at the new function below? I tested with 31.12.2013 and it works ok.

function firstMonday(year) {
  var dt = new Date(year, 0, 4);          
  return dt.setDate( dt.getDate() - (dt.getDay() || 7) +1);
}

Date.prototype.getWeekOfYear = function () {
        var week1;
        var year = this.getFullYear();
        if (this >= new Date(year, 11, 29)) {
          week1 = firstMonday(year + 1);
          if (this < week1) {
            week1 = firstMonday(year);
          } else {
                  year++;
          }
        } else {
                  week1 = firstMonday(year);
                  if (this < week1) {
                    week1 = firstMonday(--year);
                  }
        }

        return  Math.floor( (((this -week1)/86400000) / 7 + 1) );
};

@longwosion
Copy link

@FunkMonkey ,
In ISO 8601, a week begins from Monday, and Thursday is the middle day of a week.
but in jQuery.gantt, a week begins from Sunday and Wensday will be the middle day of a week.
I try to fix this issue base on this rule.

First week of a year is the week has the first Wensday.

0e70145

@taitems
Copy link
Owner

taitems commented Feb 17, 2013

The beginning of a week is cultural. I believe starting a week on a Sunday
is Germanic in origin(?), but overall, hard coding it could be dangerous.

Sent from my iPad

On 17/02/2013, at 9:13 PM, Eric SHI notifications@github.com wrote:

@FunkMonkey https://github.com/FunkMonkey ,
In ISO 8601, a week begins from Monday, and Thursday is the middle day of a
week.
but in jQuery.gantt, a week begins from Sunday and Wensday will be the
middle day of a week.
I try to fix this issue base on this rule.

First week of a year is the week has the first Wensday.

0e701450e70145


Reply to this email directly or view it on
GitHubhttps://github.com//issues/61#issuecomment-13683677.

@FunkMonkey
Copy link
Author

Well, if we go for ISO 8601, I'd say the implementation should follow the standard completely, starting the week with Monday. If you want to start the week on Sunday, then you should use the rules of the American system, but you shouldn't just mix those two different ways of calculation and end up with a completely unique calendar system.

I agree with @taitems that you should not hardcode. I guess it should be an option which calendar system to use (ISO or American), swapping the functions getWeekOfYear and getStartOfWeek (which returns Sunday or Monday).

@usmonster
Copy link
Collaborator

👍 for following standards as much as possible. I'm also inclined to believe (without any proof at all) that most folks who would use a Gantt chart at all would prefer that the week was the ISO week anyway, starting on Monday. Changing the default start-of-week might be startling to some, but I'm sure (again, without evidence) that most folks wouldn't even notice or care. Still, as @taitems said, the first day of the week is cultural, and it's more than just a Sunday vs. Monday debate: http://unicode.org/reports/tr35/tr35-dates.html#Week_Data .

As a solution, I would propose adding an optional parameter that takes a number, 0-6, to specify the first day of the week, with the number corresponding to the same day as the Date object's getDay() function (0 = Sunday, 1 = Monday, etc.). When given, this parameter should probably also be used to adjust any default dow day labels to their proper positions. If the user supplies their own dow option, however, we can probably assume that it is already in the proper order.

(Also-also, adding stuff to the Date prototype smells bad, we should probably change that..)

@taitems
Copy link
Owner

taitems commented Jul 31, 2013

The precedent for modifying the Date prototype unfortunately exists in the original code. Code smell be damned!

I'm +1 for an Int based starting day of week config option.

@usmonster usmonster added this to the v1.3.0 milestone Dec 21, 2014
usmonster added a commit that referenced this issue Dec 21, 2014
- Weeks scale now gives more realistic view of week boundaries
- All week calculations now assume ISO weeks by default (options coming soon)
- Addresses bug part of #61
- Fixes #73
- Fixes #99
- Closes #112
- Closes #153
- Use `createPseudo` for custom jquery pseudo-selector definitions when available
- Other miscellaneous incremental code cleanup and refactoring.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants