-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
Fixed styling for lines for ELK flowchart #4844
Fixed styling for lines for ELK flowchart #4844
Conversation
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also check if we can add the line length property working in elk too? (Can go in a different PR if the changes are big, no need to block this one.)
flowchart
A --> B
A ---> C
A ----> D
flowchart-elk
A --> B
A ---> C
A ----> D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please center the title.
8601e9c
to
e7e91a6
Compare
Hi @sidharthv96, Thanks for getting back to me, had to tinker around a bit, since the Elk function doesn't state its bounds right away, but I think the fixes to the title rendering function and the elk rendering order should make the title consistent. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4844 +/- ##
========================================
Coverage 44.79% 44.79%
========================================
Files 25 25
Lines 5353 5353
Branches 27 27
========================================
Hits 2398 2398
Misses 2954 2954
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. |
@sidharthv96, are we good to go? |
Hi @itsalam, @sidharthv96 is currently on vacation. It looks like there is a linting issue in your PR. Can you see if running Also, there is a merge conflict that needs to be resolved. |
2503929
to
a3bd4c4
Compare
Hi there @huynhicode, Yeah, I had earlier trouble with the pre hook commit linting and @sidharthv96 suggested skipping them. Found a workaround that should pass for now, the changes are going through now. Feel free to write back if theres anything else. |
04617eb
to
6d2904c
Compare
The E2E tests are failing for ELK. @itsalam can you please fix that? |
Is anybody still working on that? Would be so glad if the elk renderer supports the styling of links. |
* develop: (517 commits) chore: Minor fixes chore: Build docs Use develop as base on develop branch. Update renovate json update link update announcement and blog pages Remove `--force` flag Tweak editor.bash update link chore: update browsers list Update integrations-community: add Drupal and module. Support Firefox Address review comments Change run symbol feat: Make the examples interactive in the documentation site. Add langium chore: update browsers list chore(deps): update all patch dependencies chore(deps): update all minor dependencies Update keywords and description ...
* develop: Fix flowchart-elk render test chore: Add example page link in index
@itsalam, Thank you for the contribution! |
📑 Summary
Resolves #4813
📏 Design Decisions
Removed excessive padding, as on render the padding is for an empty graph and the intended graph is inserted below it.
As edgeNodeBetweenLayers already existed as a parameter, it seems like it is the most suitable parameter for adjusting line length. Also re-added working test suites to the elk flowchart to slowly introduce a working test suite.
📋 Tasks
Make sure you
MERMAID_RELEASE_VERSION
is used for all new features.develop
branch