taitems / jQuery.Gantt

jQuery Gantt Chart
http://taitems.github.io/jQuery.Gantt/
MIT License
953 stars 304 forks source link

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

Open FunkMonkey opened 11 years ago

FunkMonkey commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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

qflamol commented 11 years ago

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 commented 11 years ago

@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.

0e701451d3514df5c1b40c2a0ec34c692dcf7518

taitems commented 11 years ago

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.

0e70145https://github.com/taitems/jQuery.Gantt/commit/0e701451d3514df5c1b40c2a0ec34c692dcf7518

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

FunkMonkey commented 11 years ago

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 commented 11 years ago

:+1: 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 commented 11 years ago

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.