feat/COMPASS-9611 field to field edges #150
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gribnoysup
approved these changes
Oct 15, 2025
src/utilities/convert-edges.test.ts
Outdated
Comment on lines
147
to
158
| expect(result).toEqual({ | ||
| id: 'e1', | ||
| source: 'node1', | ||
| target: 'node2', | ||
| markerStart: 'start-many', | ||
| markerEnd: 'end-one', | ||
| type: 'fieldEdge', | ||
| data: { | ||
| sourceFieldIndex: 2, | ||
| targetFieldIndex: 4, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Super nit / just a style suggestion for the future: when writing tests like this and if possible I would highly recommend to explicitly test the values that you actually are expecting to be added / removed / modified instead of doing this sort of snapshot testing. The reason for this is that next time someone changes related code it will be really really hard to understand if the test failure happens intentionally or not, in most cases people will just opt in to just updating the snapshot negating any benefits of having the test in the first place (basically the usual troubles of having a snapshot test 🙂).
In this case for example, you can have two dedicated test cases clearly stating the intent:
- Should preserve external edge values (and test what are those if you feel like this type of test will be helpful)
- Should convert to fieldEdge if indices provided (and test the type value and actual new properties that are added)
paula-stacho
force-pushed
the
COMPASS-9611
branch
from
b364411 to
831bfe7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
External Links
COMPASS-9611
Description
Adding a new type of edges. This type will be used when the
sourceFieldIndexandtargetFieldIndexare provided. The goal is to improve readability, especially when the diagram is exported as an image.While the vertical position is fixed at the field position, the horizontal flips between left/right based on a simple heuristic:
I've been looking into how other tools solve this, for example
draw.ioflips the sides depending on the position diff, but it's always using opposite sides (doesn't apply the second part) - see screenshot. If you find a tool that has a better heuristic, let me know!Leaving self-referencing field-to-field edges out for now, might be a follow up.
📸 Screenshots/Screencasts
Storybook
Example from draw.io