feat/COMPASS-9611 field to field edges#150
Conversation
gribnoysup
left a comment
gribnoysup
left a comment
There was a problem hiding this comment.
Looks good as far as I can tell, I didn't check the math too closely, the general logic looks good on the preview and tests seem to cover it 🙂
| 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
2 participants
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