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 hex shape for flow charts #970

Merged
merged 13 commits into from
Oct 8, 2019
Merged

Conversation

mearns
Copy link
Contributor

@mearns mearns commented Oct 3, 2019

Working on issue #530.

@coveralls
Copy link

coveralls commented Oct 3, 2019

Pull Request Test Coverage Report for Build 1053

  • 66 of 75 (88.0%) changed or added relevant lines in 3 files are covered.
  • 99 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+1.9%) to 55.453%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/diagrams/flowchart/flowRenderer.js 2 3 66.67%
src/diagrams/flowchart/flowChartShapes.js 61 69 88.41%
Files with Coverage Reduction New Missed Lines %
src/diagrams/flowchart/parser/flow.jison 99 62.57%
Totals Coverage Status
Change from base Build 1023: 1.9%
Covered Lines: 2767
Relevant Lines: 4899

💛 - Coveralls

mearns added 7 commits October 2, 2019 22:23
The hexagon shape in flow chart now fits
the width and height independently, so it can better fit
large content without taking up too much space.
The "corner" triangles are a fixed ratio to the height,
so the triangles will always be mathemtically similar.
@mearns
Copy link
Contributor Author

mearns commented Oct 3, 2019

Probably won't be able to work on this until tomorrow night, but should be able to wrap it up then.

@IOrlandoni
Copy link
Member

Hey, thank you very much for the work you've done so far! Looking forward to seeing it complete and merging it!

As a side note, just in case you were not aware, when you do a pull request (even a draft) and then do commits to it, everyone that's subscribed to the repository (mostly collaborators) gets a notification (email or any other type) per each commit so, if possible, avoid doing so.

If you need to do it for a specific reason, or to make your work easier, that's more than fine! It's not a big deal!

image

@knsv
Copy link
Collaborator

knsv commented Oct 3, 2019

Nice, docs and all! Let me know when done. Will merge this to develop. Collecting changes for version 9 there. Good to have master ready for critical bug fixes.

@mearns
Copy link
Contributor Author

mearns commented Oct 3, 2019

Eek, sorry about that @dunning-kruger, didn't realize it would be so noisy. Is there any way to get coveralls to run for me without pushing commits?

@IOrlandoni
Copy link
Member

@mearns I don't think there's an easy way. I believe or current setup is building in travis then pushing the coverage to coveralls, which means that you would have to set up both in your fork.

@mearns
Copy link
Contributor Author

mearns commented Oct 5, 2019

So a little more clean up to do, but I've added some new simple tests for the flow chart shape rendering functions, which required a light refactor (specifically pulling this out into flowChartShapes.js. Wonder if the maintainers have any input on this before I go any further (really just pulling things out of the body of addToRender to satisfy the max number of lines in that function as called out by codeclimate).

@mearns
Copy link
Contributor Author

mearns commented Oct 5, 2019

I'm done with initial development on this, it's ready for review.

I implemented the hexagon shaping using double curly braces as the markup, sort of in the spirit of "round" versus "circle", but if you have an alternative you'd prefer, let me know:

A --> B{regular diamond}

vs.

A --> B{{hexagon}}

Here's a screenshot of the new element. It's designed to have the side "triangles" always a similar shape (the angles are always fixed). That means it will still grow horizontally as it grows vertically, but not nearly as aggressively as the diamond. I chose this so that the angles don't become too shallow in tall elements, which would make the hex and a rectangle harder to distinguish.

Screen Shot 2019-10-05 at 1 54 48 PM

For reference, here's the diamond with the same contents:

Screen Shot 2019-10-05 at 1 53 35 PM

I noted above a light refactor of the flowRenderer, to pull the shape-rendering functions out into a separate module. The immediate impetus for this was to make up for a decrease in code coverage that came from decreasing the total number of lines (which itself was triggered by codeclimate complaining about the same code appearing multiple times 🙁). I think the refactor is a positive change overall, but if you have other ideas just let me know =)

@mearns mearns marked this pull request as ready for review October 5, 2019 18:11
@knsv
Copy link
Collaborator

knsv commented Oct 8, 2019

Good job @mearns ! I appreciated your refactoring, it improved the code! I also like that you added many unit tests.

@knsv knsv merged commit 5541899 into mermaid-js:master Oct 8, 2019
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.

4 participants