Added exclude weekdays to definition #792

jopapo · 2019-02-06T19:18:02Z

Added: exclude weekdays [and a list of specific dates] on Gantt.

As mentioned on issue #314

It is open to discussion how to render this:

Keep this way. Extending the weekends or excluded days;
Hide the ignored dates;
Indicate the ignored dates on the task rectangle in some way.

But I think it can be handled on another issue if anyone needs it.

coveralls · 2019-02-06T19:28:27Z

Pull Request Test Coverage Report for Build 738

46 of 60 (76.67%) changed or added relevant lines in 3 files are covered.
3 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.3%) to 54.568%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/diagrams/gantt/ganttRenderer.js	0	2	0.0%
src/diagrams/gantt/parser/gantt.js	10	14	71.43%
src/diagrams/gantt/ganttDb.js	36	44	81.82%

Files with Coverage Reduction	New Missed Lines	%
src/diagrams/gantt/parser/gantt.js	1	50.4%
src/diagrams/gantt/ganttDb.js	2	71.94%

Totals
Change from base Build 716:	0.3%
Covered Lines:	2086
Relevant Lines:	3795

💛 - Coveralls

gijswijs

Great that you took it on you to tackle this. I still see some points of discussion though.

It's debatable what days are considered weekend throughout the world. Big parts of the world consider Friday and Saturday to be the weekend. So I propose to just explicitly state the weekdays you want to have excluded. So either "Saturday, Sunday" or "Friday, Saturday". This also leaves open the option for having a parttime working week where one would work 3 or 4 days a week.
This code currently seems to not take into account excluded days or dates that fall in the middle of durations. If I have a duration of 20d's I expect the Gantt chart to adjust for every excluded day within that period. Now it only seems to look at the exact end date of that duration.
If explicit end dates are given in the task definition, I don't think we should change that date if it's an excluded day or date. We should assume that the author had a valid reason for having that specific date. This is debatable, but that's what I'd would expect as behavior of the Gantt chart.

gijswijs · 2019-02-07T03:09:25Z

src/diagrams/gantt/ganttDb.js

  }
  // Default date - now
-  return d.toDate()
+  return getNextValidDate(d, dateFormat, excludes)


If I understand this correctly, this only checks whether an end date should be excluded, and if so, it returns the next Valid Date. But excluded dates can also fall in the duration of the task itself, and the duration should be adjusted accordingly. E.g. when you are excluding weekends, and the startdate is on a Friday, this function will return Tuesday as the enddate. But that's incorrect, it should return Thursday as the enddate.

Yes. It was incorrect. My fix was to target the task after the start and end computation. See below.

gijswijs · 2019-02-07T03:12:43Z

src/diagrams/gantt/ganttDb.js

-    return moment(str, dateFormat.trim(), true).toDate()
+  let mDate = moment(str, dateFormat.trim(), true)
+  if (mDate.isValid()) {
+    return getNextValidDate(mDate, dateFormat, excludes)


If an explicit enddate is set, I'm not sure whether we should change the enddate based on exclusion of weekends or other dates. You could have an explicit enddate in the weekend, because of particular reasons for that task.
The bigger question is, should enddates only be adjusted based on exclusions, when the tasks defines a duration instead of a specific enddate? I would think so.

I agree that, when the user wants an endTime, it is not necessary to skip weekend. So, my fix is to prevent this from happening.
But, I do not know if it was the best way to check if the endTime was manually inputted.

gijswijs · 2019-02-07T03:16:52Z

src/diagrams/gantt/ganttDb.js

@@ -58,7 +63,22 @@ export const getTasks = function () {
  return tasks
 }

-const getStartDate = function (prevTime, dateFormat, str) {
+const isInvalidDate = function (date, dateFormat, excludes) {
+  if (date.isoWeekday() >= 6 && excludes.indexOf('weekends') >= 0) {


Everywhere it says weekdays, (even in the PR title) but the code only checks for the word 'weekends'. The latter seems more logical, because people are more likely to wanting to exclude weekends. Although that has problems attached to it as well, which I will address in the general review comment.

Correct. I kept weekend as the "default" but if the user chooses to put the weekday name (friday, saturday, monday), it will work too.

gijswijs · 2019-02-07T03:28:53Z

src/diagrams/gantt/ganttDb.spec.js

+    ganttDb.setDateFormat('YYYY-MM-DD')
+    ganttDb.setExcludes('weekends 2019-02-06')
+    ganttDb.addSection('testa1')
+    ganttDb.addTask('test1', 'id1,2019-02-01,1d')


If this would be:

ganttDb.addTask('test1', 'id1,2019-02-01,8d')

You'd expect

expect(tasks[1].startTime).toEqual(moment('2019-02-13', 'YYYY-MM-DD').toDate())

And your code would return 2019-01-11

Some more fringe test cases are needed here.

Added more tests.

jopapo · 2019-02-07T14:10:03Z

The main reasoning now is:

if endtime is specified, there is no skipped dates;
excludes can be: dates (in the configured format), weekend (sunday and saturday), and weekdays (monday, friday). Separated by space or comma;
starttime is never computed because, or it is manually specified or is from another endtime;
each skipped date between starttime and endtime add 1 day to endtime.

I think it might be enough.

Thank you for your observations and feedback. It is long due for me to give back to the community. Im eager to learn more about it.

gijswijs · 2019-02-11T08:53:29Z

Shouldn't you check if the starttime itself isn't an excluded day or date? If a task ends on Friday, the following task should start on Monday, right? The end date should remain the same, since you already correct the enddate for the Saturday and the Sunday (if the weekend is excluded) but it just seems weird to have a task starting on a day that is excluded.

jopapo · 2019-02-11T12:38:28Z

Hum... true. It works this way, but its weird. I'll take a look on this.

jopapo · 2019-02-12T19:33:23Z

Created and renderEndDate to achieve those skipped weekends on chart.
Took the liberty to attach a the test print to easy visualization.

pgebheim · 2019-05-28T14:54:52Z

What are the chances this can be merged?

Would love to get this feature upstream into all the VSCode plugins and such.

knsv · 2019-05-28T19:59:04Z

If it works the chances are good!

pgebheim · 2019-05-28T20:27:55Z

Amazing!

…yarn/develop/mermaid-9.1.1 chore(deps): bump mermaid from 9.0.1 to 9.1.1

João Paulo Poffo added 7 commits February 6, 2019 16:54

Added excludes weekdays to gantt

418ac50

Docs

5a9c57e

Doc HTML

997cdff

Standard

a8e9f21

Comments

8e8651a

Codeclimate - Complexity

9f1c37e

Standard JS

1d04c7e

gijswijs suggested changes Feb 7, 2019

View reviewed changes

João Paulo Poffo added 4 commits February 7, 2019 11:21

Fixed weekend between dates; Manual endtimes; Additional testing

8a8b7bd

Standard

a99b31a

Complexity

5bfddcc

Standard

58df729

Defined a renderEndDate

6e846ac

João Paulo Poffo added 2 commits February 12, 2019 17:40

Simplify to codeclimate

8ce6584

Simplified methods

afeb3b5

knsv merged commit 9191663 into mermaid-js:master May 28, 2019

JoanZapata mentioned this pull request Jul 25, 2019

Gnatt Chart exclude weekends #314

Closed

mgenereu pushed a commit to mgenereu/mermaid that referenced this pull request Jun 25, 2022

Merge pull request mermaid-js#792 from mermaid-js/dependabot/npm_and_…

4c63c08

…yarn/develop/mermaid-9.1.1 chore(deps): bump mermaid from 9.0.1 to 9.1.1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added exclude weekdays to definition #792

Added exclude weekdays to definition #792

jopapo commented Feb 6, 2019

coveralls commented Feb 6, 2019 •

edited

Loading

gijswijs left a comment

gijswijs Feb 7, 2019

jopapo Feb 7, 2019

gijswijs Feb 7, 2019

jopapo Feb 7, 2019 •

edited

Loading

gijswijs Feb 7, 2019

jopapo Feb 7, 2019

gijswijs Feb 7, 2019

jopapo Feb 7, 2019 •

edited

Loading

jopapo commented Feb 7, 2019 •

edited

Loading

gijswijs commented Feb 11, 2019

jopapo commented Feb 11, 2019

jopapo commented Feb 12, 2019 •

edited

Loading

pgebheim commented May 28, 2019

knsv commented May 28, 2019

pgebheim commented May 28, 2019

Added exclude weekdays to definition #792

Added exclude weekdays to definition #792

Conversation

jopapo commented Feb 6, 2019

coveralls commented Feb 6, 2019 • edited Loading

Pull Request Test Coverage Report for Build 738

💛 - Coveralls

gijswijs left a comment

Choose a reason for hiding this comment

gijswijs Feb 7, 2019

Choose a reason for hiding this comment

jopapo Feb 7, 2019

Choose a reason for hiding this comment

gijswijs Feb 7, 2019

Choose a reason for hiding this comment

jopapo Feb 7, 2019 • edited Loading

Choose a reason for hiding this comment

gijswijs Feb 7, 2019

Choose a reason for hiding this comment

jopapo Feb 7, 2019

Choose a reason for hiding this comment

gijswijs Feb 7, 2019

Choose a reason for hiding this comment

jopapo Feb 7, 2019 • edited Loading

Choose a reason for hiding this comment

jopapo commented Feb 7, 2019 • edited Loading

gijswijs commented Feb 11, 2019

jopapo commented Feb 11, 2019

jopapo commented Feb 12, 2019 • edited Loading

pgebheim commented May 28, 2019

knsv commented May 28, 2019

pgebheim commented May 28, 2019

coveralls commented Feb 6, 2019 •

edited

Loading

jopapo Feb 7, 2019 •

edited

Loading

jopapo Feb 7, 2019 •

edited

Loading

jopapo commented Feb 7, 2019 •

edited

Loading

jopapo commented Feb 12, 2019 •

edited

Loading