Skip to content

Conversation

@AntonJames-Sistence
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A
GET-34 & GET-35

What is the new behavior?

render_order.mov

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@netlify
Copy link

netlify bot commented Sep 24, 2025

Deploy Preview for reagraph ready!

Name Link
🔨 Latest commit ab85078
🔍 Latest deploy log https://app.netlify.com/projects/reagraph/deploys/68d569bb880f840008f4b3d3
😎 Deploy Preview https://deploy-preview-384--reagraph.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 project configuration.

if (isOrthographic && cameraRef.current) {
// Convert distance to zoom values (inverse relationship)
cameraRef.current.maxZoom = maxDistance
? 50000 / minDistance
Copy link
Contributor

Choose a reason for hiding this comment

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

@AntonJames-Sistence Should we allow modification of this constant? Maybe better to add this as props with a default value to have the ability to change it if needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SerhiiTsybulskyi we can modify maxDistance/minDistance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok I think I need a better idea for this min/max zoom, because it actually doesn't work as expected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SerhiiTsybulskyi I had to add min/max zoom props, since there's no actual correlation between zoom and dolly, as I thought in the beginning

Copy link
Member

@amcdnl amcdnl 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 overall but a few minor nits.

@amcdnl amcdnl requested a review from Copilot September 25, 2025 15:39
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses rendering order issues and orthographic camera zoom limits. It introduces customizable render order for line elements and fixes orthographic camera min/max zoom behavior.

Key changes:

  • Add configurable render order support for Line and Edge components
  • Fix orthographic camera zoom limits by converting distance parameters to zoom values
  • Improve visual layering for active/selected edges

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/symbols/Line.tsx Adds renderOrder prop with default value of -1
src/symbols/Edge.tsx Updates z-position logic and applies dynamic renderOrder based on state
src/CameraControls/CameraControls.tsx Implements orthographic camera zoom limits using distance-to-zoom conversion

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 290 to 295
cameraRef.current.maxZoom = maxDistance
? 50000 / minDistance
: undefined;
cameraRef.current.minZoom = minDistance
? 50000 / maxDistance
: undefined;
Copy link

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

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

The magic number 50000 is used for zoom calculations without explanation. Consider extracting this as a named constant with a descriptive comment explaining its purpose in the distance-to-zoom conversion formula.

Copilot uses AI. Check for mistakes.
Comment on lines 291 to 294
? 50000 / minDistance
: undefined;
cameraRef.current.minZoom = minDistance
? 50000 / maxDistance
Copy link

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

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

The zoom limit calculation uses minDistance in the denominator when maxDistance exists, but this should use minDistance for maxZoom calculation. The logic appears reversed - maxZoom should be calculated from maxDistance, not minDistance.

Suggested change
? 50000 / minDistance
: undefined;
cameraRef.current.minZoom = minDistance
? 50000 / maxDistance
? 50000 / maxDistance
: undefined;
cameraRef.current.minZoom = minDistance
? 50000 / minDistance

Copilot uses AI. Check for mistakes.
@AntonJames-Sistence AntonJames-Sistence merged commit 1192f49 into master Sep 25, 2025
5 checks passed
@AntonJames-Sistence AntonJames-Sistence deleted the anton/render-order branch September 25, 2025 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants