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

refactor: Unify the edgeMarker adding logic #4837

Merged
merged 8 commits into from
Nov 27, 2023

Conversation

sidharthv96
Copy link
Member

@sidharthv96 sidharthv96 commented Sep 14, 2023

📑 Summary

Simplify and unify the logic to add edgeMarker

📏 Design Decisions

Keeping it separate will add more bugs when a PR modifies only one place.

📋 Tasks

Make sure you

@netlify
Copy link

netlify bot commented Sep 14, 2023

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit b5ba095
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/65603825158d700008f3135d
😎 Deploy Preview https://deploy-preview-4837--mermaid-js.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added the Type: Other Not an enhancement or a bug label Sep 14, 2023
@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Merging #4837 (b5ba095) into develop (8f733c6) will decrease coverage by 1.91%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4837      +/-   ##
===========================================
- Coverage    77.35%   75.44%   -1.91%     
===========================================
  Files          164      165       +1     
  Lines        13869    13845      -24     
  Branches       698      702       +4     
===========================================
- Hits         10728    10446     -282     
- Misses        2987     3228     +241     
- Partials       154      171      +17     
Flag Coverage Δ
e2e 80.40% <100.00%> (-2.37%) ⬇️
unit 42.91% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
packages/mermaid/src/dagre-wrapper/edgeMarker.ts 100.00% <100.00%> (ø)
packages/mermaid/src/dagre-wrapper/edges.js 80.00% <100.00%> (-1.19%) ⬇️

... and 10 files with indirect coverage changes

Copy link
Member

@aloisklink aloisklink left a comment

Choose a reason for hiding this comment

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

Looks good! This will make the code a lot more maintainable! The only issue I see is that the current code ignores invalid values (like ) url(https://my-malicious-url.example), but your proposed code adds a marker with that value.

My recommendation is to either:

  • keep the old behavior and ignore invalid values (see my comment for details), or
  • if we know that invalid values will never be used, we should probably change the TypeScript type from string to "arrow_cross" | "arrow_point" | "aggregation" // etc.

packages/mermaid/src/dagre-wrapper/edgeMarker.ts Outdated Show resolved Hide resolved
sidharthv96 and others added 4 commits November 9, 2023 02:37
Co-authored-by: Alois Klink <alois@aloisklink.com>
* develop: (155 commits)
  chore(deps): update all patch dependencies
  chore: release v10.6.1
  fix(flow): fix invalid ellipseText regex
  review fixes
  Update XYChart's nav link in the docs template
  add comment for ts ignore
  move decodeEntities to utils
  review fixes
  chore(deps): update all minor dependencies
  chore: Point to correct changelog
  add spec
  fix: getMessageAPI so it considers entity codes
  chore(deps): update all patch dependencies
  Update integrations-community.md
  docs: upate the list of tools with native support of mermaid
  Fix typo in build-docs.yml
  Updated mermaid version
  Limiting the number of edges that are allowed in the flowchart
  Update README.md
  Update README.md
  ...
Copy link
Member

@aloisklink aloisklink left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes. It all looks good to me :).

I think you're good to merge now, but before we merge, my gut feeling is to add a Skip changelog label to this PR, since it seems like a lowish risk refactoring change. But 🤷, it's up to you.

Edit: At some point, we probably should write a "Reviewing PRs" guide for maintainers, including when to put labels on PRs.

@sidharthv96 sidharthv96 added the Skip changelog Don't include in the changelog label Nov 10, 2023
@sidharthv96 sidharthv96 added this pull request to the merge queue Nov 25, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 25, 2023
@aloisklink aloisklink added this pull request to the merge queue Nov 27, 2023
Merged via the queue into develop with commit 25e9bb3 Nov 27, 2023
26 checks passed
@aloisklink aloisklink deleted the refactor/unifyEdgeMarkers branch November 27, 2023 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip changelog Don't include in the changelog Type: Other Not an enhancement or a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants