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

Convert gitGraph to TS #5407

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

NicolasCwy
Copy link
Contributor

📑 Summary

Brief description about the content of your PR.

Resolves #

📏 Design Decisions

Describe the way your implementation works or what design decisions you made if applicable.

📋 Tasks

Make sure you

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

netlify bot commented Mar 23, 2024

Deploy Preview for mermaid-js failed.

Name Link
🔨 Latest commit d464a6b
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/65feb85b92a8220008dbcf34

@NicolasCwy
Copy link
Contributor Author

@sidharthv96 Hey sidharth would you know if this .hash is used? I don't see this for other diagrams so I have a feeling this is legacy code that I can remove

const error = new Error('Incorrect usage of "merge". Cannot merge a branch to itself');
error.hash = {
text: 'merge ' + otherBranchName,
token: 'merge ' + otherBranchName,
line: '1',
loc: { first_line: 1, last_line: 1, first_column: 1, last_column: 1 },
expected: ['branch abc'],
};

@sidharthv96
Copy link
Member

Yes, you can remove that hash.

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.

Awesome work so far @NicolasCwy 🚀
Some minor comments regarding the conventions.

const orderInt = parseInt(order, 10);
if (isNaN(orderInt)) {
throw new Error('Invalid order provided');
// TODO - what is this hash?
Copy link
Member

Choose a reason for hiding this comment

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

The hash is used in external apps like the live editor to show more info about an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh so in your previous comment about removing that hash, you didn't mean that I can remove all the hashes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bump on this @sidharthv96

Copy link
Member

Choose a reason for hiding this comment

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

You can remove the ones that are incorrect/filled with dummy data. It would be better if you can put the right data in, but I don't think that's available in the DB. (line position and all)

Comment on lines +281 to +288
'merge ' +
otherBranchName +
' ' +
custom_id +
'_UNIQUE ' +
override_type +
' ' +
custom_tag,
Copy link
Member

Choose a reason for hiding this comment

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

Please use string template instead.

@NicolasCwy
Copy link
Contributor Author

@sidharthv96 Can the old git graph renderer be removed? I don't see any references to it

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.

None yet

2 participants