taitems / jQuery.Gantt

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

Generating a date width will fail if an invalid date value is entered first. #239

Closed tkmry closed 3 years ago

tkmry commented 3 years ago

If the first object in the "source" array passed to jQuery.gantt contains incorrect date information, the calendar will fail to generate (NaN/Undefined). These are caused by the getMinDate and getMaxDate implementations. When the getMinDate or getMaxDate function receives incorrect date information for the first loop, the minDate/maxDate variable is assigned "Invalid Date", but the value does not change in the loop and minDate/maxDate has "Invalid Date" remains set.

I've come up with a code that solves the above problem for my personal requirements and posted a link (*Ref1) to a modified version of the function in question. I am aware that this is a problem with jquery.gantt itself, so I am putting it up as an issue, but should I send a pull request?

*Ref1 ガントチャートプラグイン – cybozu developer network https://developer.cybozu.io/hc/ja/articles/203716110?page=1#comment_900001456206


in Origin(jp)

jQuery.gantt に渡した "source" 配列の最初のオブジェクトが適切でない日付情報を保持していると、カレンダーの生成に失敗(NaN/Undefined)する。 これらはgetMinDateやgetMaxDateの実装により引き起こされている。 getMinDateやgetMaxDate関数が1ループ目の処理に不適切な日付情報を受け取ると、minDate/maxDate 変数は "Invalid Date" を代入されるが、ループの中で値が変わることはなく minDate/maxDate には "Invalid Date" がセットされたままになる。

個人的な要件で上記の問題を解決するコードを考え、該当関数の修正内容をリンク先(*Ref1)に投稿した。 この問題は jquery.gantt 自体の問題だと認識しているため、一旦issueとして立てているが、pull requestを送った方が良いだろうか?

*Ref1 ガントチャートプラグイン – cybozu developer network https://developer.cybozu.io/hc/ja/articles/203716110?page=1#comment_900001456206

tkmry commented 3 years ago

The snippet of this issue. Check it, please.

// Common Source
    var valid_source = {
        name: "valid Date",
        values: [{
            from: "/Date(2020/10/11)/",
            to: "/Date(2020/10/12)/",
            label: "has Date",
        }]
    }
    var invalid_source  = {
        name: "Invalid Date",
        values: [{
            from: "",
            to: "",
            label: "text",
        }]
    };

Pattern1.

// Correct
    $(".gantt").gantt({
        source: [
            valid_source,
            invalid_source
        ],
        scale: "days",
    });

image

Pattern2.

// Wrong
    $(".gantt").gantt({
        source: [
          invalid_source,
          valid_source
        ],
        scale: "days",
    });

image

(2020/11/23) The "invalid date" and "valid date" for "source" were reversed in name and value, so this has been corrected. And chagne screen shots.

usmonster commented 3 years ago

Hi @tkmry! Thanks for using the plugin and reporting an issue here.

If I understand correctly, the issue is that the chart rendering breaks if invalid data is provided. If this is the case, then the plugin is working as designed—we prefer to "fail fast" when invalid data is given, though it's true that it might be nicer to fail more gracefully.

Still, in this case, I will close the issue and the related PR #241, if you don't mind. Please feel free to reopen the issue if I've misunderstood the issue.

Thanks again for using the plugin! (And please be sure you're using the version provided in this repo, as it has behaviors that are a bit different from the upstream repo.)

tkmry commented 3 years ago

Thank you for respond.

I am checking the operation of this repository, which is currently provided, rather than the upstream repository. Sorry for not including version information.

This case is not a problem that occurs when "invalid data is provided". It's a problem that occurs "when invalid data is provided 'at the first'". If by design the correct behavior is to break the rendering when "invalid data is provided", then the rendering should also be broken in Pattern1 and Pattenr2 as mentioned above. But personally, I don't want the rendering to be broken in either case.

Translated with www.DeepL.com/Translator (free version)

tkmry commented 3 years ago

Hi, @usmonster. I think, this issue is not resolved. Reopen, please.

usmonster commented 3 years ago

Indeed, it would be better for the behavior to be consistent in any case, so I'll reopen the issue.

Can you please send me a link to a page with a working example that demonstrates the issue?

The screenshots and code are helpful, but it's much easier if you send me something like a JSFiddle or CodePen link.

Thanks!

tkmry commented 3 years ago

Thank you for reopening. I wrote the code for this case here, so take a look. http://jsfiddle.net/3awxvbLs/4/ (forked from hxxp://jsfiddle.net/jleviaguirre/vL8pP/)

tkmry commented 3 years ago

sample2.

fail all if invalid date value provided. http://jsfiddle.net/1m42pgh3/

Somehow generating if invalid date value provided. http://jsfiddle.net/xt3bckgm/2/

usmonster commented 3 years ago

Thanks for the live demo! I can now more clearly see what the issue is.

Since the plugin doesn't do strict input validation, this can lead to invalid Date objects being passed around. This is "fine" since we don't define what we do with invalid input, but the way we determine the bounds of the chart can lead to inconsistent rendering behavior, as already described.

After reviewing the affected code, I think the approach you suggested in #241 is a good solution, so I'll reopen it.

Thanks again for contributing!

tkmry commented 3 years ago

Thanks for checking and review! My problem is solved, I'll close this issue. <3

tkmry commented 3 years ago

Sorry, I misunderstood and closed this issue before the merge. When I get home from work, I will correct it and send you another PR as per your review.

usmonster commented 3 years ago

Thanks, @tkmry! You don't need to open a new PR, as it's easy to apply the suggestions in the review. I can actually apply the suggestions myself and merge the PR.

usmonster commented 3 years ago

It's been merged, so I will close this issue. Thanks again for your contribution!