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

Gantt: Add support for callback with parameters #1136

Merged
merged 4 commits into from
Jan 1, 2020

Conversation

keenanjt33
Copy link
Contributor

This is meant to address: #921

After researching this issue I agreed with Dunning-Kruger that it seemed prudent to support passing in the node identifier as the callback parameter as a solution. I then found that the project already supports this for flowcharts. Gantt charts, on the other hand, already support passing in custom parameters to a callback function. This, however, was not reflected in the documentation.

For continuity between flowchart and Gantt, I added in code to automatically pass in the task identifier to the callback function if no custom callback parameters are specified. I also added interaction documentation based off of the documentation for flowchart and #804.

This would be my first contribution so any feedback would be appreciated.

@IOrlandoni IOrlandoni requested a review from knsv December 11, 2019 13:26
@klemmchr klemmchr changed the title Feature #921: Callback with parameters Gantt: Add support for callback with parameters Dec 11, 2019
@klemmchr klemmchr added Graph: Gantt Type: Enhancement New feature or request labels Dec 11, 2019
@knsv
Copy link
Collaborator

knsv commented Dec 11, 2019

@keenanjt33 Thanks Keenan! Great that you added docs and tests! We have a browser suite using cypress and it would be neat to have this change covered in the integration tests. If you add that I would be happy to merge this PR

Path:
cypress/integration/other/interaction.spec.js

to run, do yarn devto start the dev server. Then in another command prompt do cypress open.

Copy link
Collaborator

@knsv knsv left a comment

Choose a reason for hiding this comment

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

@keenanjt33 Thanks Keenan! Great that you added docs and tests! We have a browser test suite using cypress and it would be neat to have this change covered in the integration tests. If you add that I would be happy to merge this PR

Path:
cypress/integration/other/interaction.spec.js

to run, do yarn devto start the dev server. Then in another command prompt do cypress open.

@knsv knsv merged commit a162692 into mermaid-js:develop Jan 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Graph: Gantt Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants