Skip to content

Conversation

@artemkosenko
Copy link
Contributor

Hello there.

I propose two things:

  1. An additional boolean configuration parameter 'adjustOverlaps'.
    The reason for that is that I have more 'out-connections' than 'in-connections'. (basically elements may fall into multiple categories)
    image
    It doesn't look nice.
    So the 'adjustOverlaps === true' param does the following:
    image

By default it is set to 'false'. The usual logic stays the same.

  1. An updated way to calculate the largest node.
    This is a controversian one. But it helped me.
    The problem was due to the described above thing - more 'out-connections' than 'in-connections'.
    The 'findLargestNode' returned a node from a middle which then led to a visual bug.
    It got:
    image
    But i expected one of the images above.

I added the 'tree.html' file which can reproduce the issue if the 'findLargestNode' function is reverted back.
Also there is a test for that.
expect(oneThenTwo).toEqual(jasmine.objectContaining({y: 28}));
It failed here because 'y' was undefined;

The controvercial thing is that a few tests are broken because charts do not conform to the png images provided.
I'd be nice you if could help with those images.

Thanks

- added 'adjustOverlaps' parameter
- updated the 'findLargestNode' to prioritize nodes with no in-connections
- updated README.md
- added a test for complex flow which failed to draw
- added the tree.html file to test the latest changes
@artemkosenko
Copy link
Contributor Author

I realized that I needed to use node priorities to get what i wanted.
So I reverted back the second change.

Copy link
Owner

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

I don't like the adjustOverlap as a option name.

What do you think about size: 'min' (default 'max')? Should work both ways and can be determined like Math[options.size](node.in, node.out)

That would determine the node height. The flows would then be aligned using a simple multiplier (in/height or out/height), which can always be used, and should be limited to be <=1.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@kurkle kurkle added documentation Improvements or additions to documentation enhancement New feature or request and removed documentation Improvements or additions to documentation labels Jul 30, 2021
Copy link
Owner

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

I'll need to do some testing before releasing this, but I'll be away for couple of weeks. So be patient.

@kurkle kurkle changed the title Improvements Add size option, update readme Jul 30, 2021
@artemkosenko
Copy link
Contributor Author

Sure. Take you time.

@kurkle kurkle added this to the 0.8.0 milestone Sep 1, 2021
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 1, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@kurkle kurkle merged commit 3747975 into kurkle:master Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants