Skip to content

Factor in arrow radius and padding for SVG sizing#24

Merged
bvisness merged 1 commit intomozilla-spidermonkey:mainfrom
aidenfoxivey:afi-fix-svg-dimensions
Dec 2, 2025
Merged

Factor in arrow radius and padding for SVG sizing#24
bvisness merged 1 commit intomozilla-spidermonkey:mainfrom
aidenfoxivey:afi-fix-svg-dimensions

Conversation

@aidenfoxivey
Copy link
Contributor

@aidenfoxivey aidenfoxivey commented Nov 13, 2025

Fixes #23

I essentially just factor in adding the size of the CONTENT_PADDING and ARROW_RADIUS to the current y or x position. Then I take whatever is the greater of {y,x}max and this variable.

I think this is an appropriate way to solve this, but I'm quite open to refactoring if it's not quite the right approach.

@bvisness
Copy link
Collaborator

Thanks for catching this. I don't really like how ad-hoc this fix is though, for example you set the height of the SVG twice. I'd like to do this a bit more robustly.

At the very least, I would move the setAttribute(width/height) down to the bottom of the function when we're totally done with the SVG. I think then I would maybe add some kind of utility called trackPositions(xs, ys) that does the max-ing in a quick way. Then finally you could do something like this in all the arrow-drawing paths:

const x2 = dst.pos.x + PORT_START;
const y2 = dst.pos.y;
const ym = (y1 - node.size.y) + layerHeights[layer] + TRACK_PADDING + trackHeights[layer] / 2 + node.jointOffsets[i];
const arrow = downwardArrow(x1, y1, x2, y2, ym, dst.block !== null);
svg.appendChild(arrow);
trackPositions([x1, x2], [y1, y2, ym]); // updates maxX and maxY with the given x's and y's

That keeps the logic concise and close to the places where we draw, and gives me more peace of mind that we won't miss any other wacky paths in the future.

@aidenfoxivey
Copy link
Contributor Author

Thanks for catching this. I don't really like how ad-hoc this fix is though, for example you set the height of the SVG twice. I'd like to do this a bit more robustly.

At the very least, I would move the setAttribute(width/height) down to the bottom of the function when we're totally done with the SVG. I think then I would maybe add some kind of utility called trackPositions(xs, ys) that does the max-ing in a quick way. Then finally you could do something like this in all the arrow-drawing paths:

const x2 = dst.pos.x + PORT_START;
const y2 = dst.pos.y;
const ym = (y1 - node.size.y) + layerHeights[layer] + TRACK_PADDING + trackHeights[layer] / 2 + node.jointOffsets[i];
const arrow = downwardArrow(x1, y1, x2, y2, ym, dst.block !== null);
svg.appendChild(arrow);
trackPositions([x1, x2], [y1, y2, ym]); // updates maxX and maxY with the given x's and y's

That keeps the logic concise and close to the places where we draw, and gives me more peace of mind that we won't miss any other wacky paths in the future.

Seems reasonable - that's probably a better choice.

@aidenfoxivey aidenfoxivey reopened this Nov 19, 2025
@aidenfoxivey
Copy link
Contributor Author

I think this might be closer to ideal. Please let me know if I misinterpreted anything.

@bvisness
Copy link
Collaborator

bvisness commented Dec 2, 2025

Looks great! Sorry for the delay here.

@bvisness bvisness merged commit 1eb9715 into mozilla-spidermonkey:main Dec 2, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

SVG box sizing cuts off arrow.

2 participants