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

Add Custom Date Range to Gantt Charts Part 2 #5220

Open
wants to merge 35 commits into
base: develop
Choose a base branch
from

Conversation

paldeep-singh
Copy link

@paldeep-singh paldeep-singh commented Jan 20, 2024

📑 Summary

This PR is a duplicate of #4563, to resolve #1530. I am slightly desperate for this feature and did not want to trouble @AlexMooney for permission to fork his repo so I have simply copied his changes over into a new branch. Big thanks to him for letting me take over and for doing most of the work!

Most of the changes included in this PR are Alex's work, I have only done the following:

  • Changed logic for determining start date to take an end-only date range into account
    • This fixes the failures on the e2e tests for Actual date, Only start and Only end that Alex wrote in response to the one outstanding comment on his PR.
    • I have also added the documentation for verifying snapshots to the contribution docs, and had a look at the snapshots in Cypress to verify them (would appreciate a second set of eyes though since I this is my first contribution).
  • Fixed the failing ganttDb unit test

Resolves #1530

📏 Design Decisions

[x] Remove all tasks which are completely outside of the custom range
[x] Force the x-range on the plot to match the custom range (though there might be some rounding)
[x] Allow setting an absolute range dateRange :2023-06-01, 2023-07-01
[ ] Allow setting a relative range dateRange :today - 7d, today + 30d (Alex suggested leaving this for future PRs and I tend to agree)
[x] Allow setting only lower bound dateRange :2023-06-01
[x] Allow setting only upper bound dateRange :,2023-07-01 👈 kind of strange; proposals welcomed
[x] Tests for the range values
[x] Manual testing with some screen shots (As mentioned above, would appreciate a second set of eyes since I this is my first contribution)
[x] Remove backup logic for parsing date with new Date() if dayjs determines date provided is invalid.
[x] Set default for dateFormat to YYYY-MM-DD to prevent breaking charts due to above change if no dateFormat is provided. This change brings the code in line with the documentation
[x] Fixed syntax for title and dateFormat in theme cypress tests. This has caused the snapshots test to fail. I have manually checked the new screenshots and the rendered chart is accurate to the diagram specified in the tests.

Possible problems

  • The dateRange is interpreted with the dateFormat, so if the former comes first, it'll can screw up the date parsing
    • This was mentioned on the other PR but not as a blocker from what I could tell, let me know if it is and I can look into it
  • Changing the default for dateFormat to YYYY-MM-DD may break charts that are using the ISO 8601 extended date format YYYY-MM-DDTHH:mm:ss.sss without specifying a dateFormat. This would previously have been parsed by the (now removed) backup logic using new Date() which would run if dayjs deemed the provided date to be invalid. See comment.

📋 Tasks

Make sure you

Copy link

netlify bot commented Jan 20, 2024

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit d481122
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/66b58ab136cdd900089bee23
😎 Deploy Preview https://deploy-preview-5220--mermaid-js.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added the Type: Enhancement New feature or request label Jan 20, 2024
Copy link

codecov bot commented Jan 20, 2024

Codecov Report

Attention: Patch coverage is 93.90244% with 5 lines in your changes missing coverage. Please review.

Project coverage is 5.99%. Comparing base (47601ac) to head (d481122).

Files Patch % Lines
packages/mermaid/src/diagrams/gantt/ganttDb.js 95.06% 4 Missing ⚠️
...ckages/mermaid/src/diagrams/gantt/ganttRenderer.js 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #5220      +/-   ##
==========================================
+ Coverage     5.85%   5.99%   +0.14%     
==========================================
  Files          274     274              
  Lines        41112   41167      +55     
  Branches       488     517      +29     
==========================================
+ Hits          2408    2470      +62     
+ Misses       38704   38697       -7     
Flag Coverage Δ
unit 5.99% <93.90%> (+0.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...ckages/mermaid/src/diagrams/gantt/ganttRenderer.js 0.00% <0.00%> (ø)
packages/mermaid/src/diagrams/gantt/ganttDb.js 78.88% <95.06%> (+1.40%) ⬆️

@paldeep-singh paldeep-singh changed the title Feature/1530 gantt chart custom range 2 Add Custom Date Range to Gantt Charts Part 2, resolves #1530 Jan 21, 2024
@paldeep-singh paldeep-singh changed the title Add Custom Date Range to Gantt Charts Part 2, resolves #1530 Add Custom Date Range to Gantt Charts Part 2 #1530 Jan 21, 2024
@paldeep-singh paldeep-singh changed the title Add Custom Date Range to Gantt Charts Part 2 #1530 Add Custom Date Range to Gantt Charts Part 2 Jan 21, 2024
@paldeep-singh paldeep-singh marked this pull request as ready for review January 21, 2024 03:27
@paldeep-singh
Copy link
Author

For the changes to the GanttDb.js and GattDb.spec.js files, I had to commit without running the pre-commit hooks as ESLint kept crashing due to Node running out of heap memory, is that a major problem?

@sidharthv96
Copy link
Member

I will review the PR soon, but mentioning some ways to share credit to original authors when forking.

  1. Add their fork as another origin, create a new branch, add your commits, push to your fork.
  2. Add them as Co-Authors.

@paldeep-singh
Copy link
Author

I will review the PR soon, but mentioning some ways to share credit to original authors when forking.

1. Add their fork as [another origin](https://docs.github.com/en/get-started/getting-started-with-git/managing-remote-repositories), create a new branch, add your commits, push to your fork.

2. Add them as [Co-Authors](https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors).

Thanks, I did try #1, but could not pull the branch, will make sure to add a co-author to commits in the future!

Copy link
Member

@sidharthv96 sidharthv96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @AlexMooney and @paldeep-singh. We need some more test cases and tighten up the syntax.

Comment on lines 29 to 33
it('should handle a dateRange definition', function () {
const str = 'gantt\ndateRange : 2023-06-01, 2023-07-01';

expect(parserFnConstructor(str)).not.toThrow();
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need more test cases here.

And the behavior below is flaky. The format mentioned in the docs is not tested here.
So we should make it strict, then test and document it well.

dateRange 13:00, 14:00 fails
dateRange 13:00, 14:00 passes (two spaces)
dateRange: 13:00, 14:00 fails
dateRange : 13:00, 14:00 passes

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made it more strict by removing the logic that would attempt to parse the date using new Date after dayjs.isValid() returns false (see this commit). This change has not broken any tests, but I suspect it might break existing charts that are passing an invalid dateFormat while still successfully rendering due to that "backup" logic.

For valid date ranges, I've now included a test with a date-only range, a time only range, and a date+time range. Are there any other test cases I should add?

For the tests for invalid date ranges, I realised that depending on the specific error in syntax, the dateRange declaration could either be ignored or throw an error.

From your examples above, dateRange: 13:00, 14:00 would be ignored since the dateRange: (without a space before the colon) would not be considered a match by the jison parser looking for "dateRange"\s[^#\n;], and the dateRange would remain as the default of ''. For dateRange : 13:00, 14:00 (space before colon), the declaration would be picked up, but an error will be thrown as : 13:00 would be considered an invalid date. I've added tests for both these scenarios (see this commit), and would appreciate any feedback!

cypress/integration/rendering/gantt.spec.js Outdated Show resolved Hide resolved
packages/mermaid/src/diagrams/gantt/ganttDb.js Outdated Show resolved Hide resolved
@paldeep-singh
Copy link
Author

@sidharthv96 Thanks for the review! I'll address the comments over the weekend

@paldeep-singh
Copy link
Author

Sorry, just been a bit busy recently, should have time in the next couple of weeks

@paldeep-singh paldeep-singh force-pushed the feature/1530_gantt_chart_custom_range_2 branch from 6980209 to ec5b9b2 Compare March 17, 2024 03:32
@paldeep-singh
Copy link
Author

@sidharthv96 I have addressed your comments. There are 5 failing e2e tests, 1 is meant to fail and the other 4 are failures in the theme tests. I had to fix the syntax for dateFormat specified in the tests which has caused the chart to render differently. However, I have manually checked the new screenshots and they are accurate to the gantt diagram specified in the tests. Prior to the changes I made, the first task in the diagram wasn't rendered correctly, now it is and as such, dependent tasks have moved to the appropriate position on the chart.

@sidharthv96 sidharthv96 mentioned this pull request Mar 25, 2024
12 tasks
@sidharthv96
Copy link
Member

@paldeep-singh can you fix the merge conflict please?

@paldeep-singh
Copy link
Author

@paldeep-singh can you fix the merge conflict please?

Sorry, missed that one, done now!

@paldeep-singh
Copy link
Author

@sidharthv96 Is there anything else in this PR you'd like addressed?

Copy link

argos-ci bot commented Aug 9, 2024

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) ⚠️ Changes detected (Review) 1 changed, 9 added, 1 removed, 1 failure Aug 9, 2024, 3:30 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gnatt Chart Custom Date Range
2 participants