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

visjs-network#39: add penwidth support for dot language #24

Merged
merged 3 commits into from
Jul 28, 2019

Conversation

mojoaxel
Copy link
Member

@mojoaxel mojoaxel commented Jul 26, 2019

This is an original contribution from @geminoavisjs-community/visjs-network#39

part of #7


This series of patches is for adding 'penwidth' attribute for dot parser which defines the line width as described in https://www.graphviz.org/doc/info/attrs.html#d:penwidth. It includes refactoring of parseAttributeList() for reducing the number of lines to avoid max-statements limitation of lint.

Copy link
Member Author

@mojoaxel mojoaxel left a comment

Choose a reason for hiding this comment

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

It works! see examples/network/data/dotLanguage/dotEdgeStyles.html
image

mojoaxel pushed a commit that referenced this pull request Jul 27, 2019
'penwidth' defines the line width as 'width'. If both are exist,
'penwidth' is used and 'width' is discarded as described in [1].
This update fixes #24.

[1] https://www.graphviz.org/doc/info/attrs.html#d:penwidth
To reduce the number of statements of parseAttributeList(), move parsing
'dir' attribute to function parseDirAttribute() because it is going to
reach max-statements limitation of lint.
'penwidth' defines the line width as 'width'. If both are exist,
'penwidth' is used and 'width' is discarded as described in [1].
This update fixes #24.

[1] https://www.graphviz.org/doc/info/attrs.html#d:penwidth
@mojoaxel
Copy link
Member Author

I think this is ready to be merged! The screenshot shows that it worked, but feel free to check it yourself before merging. Thanks!

@mojoaxel mojoaxel added this to the v6 milestone Jul 27, 2019
@mojoaxel
Copy link
Member Author

@Thomaash Can you please have a look at this. I think we can merge this!

Copy link
Member

@Thomaash Thomaash left a comment

Choose a reason for hiding this comment

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

LGTM. A test would be nice though.

@mojoaxel
Copy link
Member Author

LGTM. A test would be nice though.

This example has been updated:
examples/network/data/dotLanguage/dotEdgeStyles.html

@Thomaash
Copy link
Member

Yeah. But it doesn't have the beauty of running npm test.

@mojoaxel
Copy link
Member Author

Yeah. But it doesn't have the beauty of running npm test.

I have no ide how to test something like this without a "visual" test. See also #33 for this.
Feel free to provide a PR to this PR with a test. Writing tests is not exactly my strength.

@mojoaxel
Copy link
Member Author

@Thomaash @yotamberk Can somebody please merge this so that I can create the next big release. Thx!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants