Skip to content

Conversation

@adamchalmers
Copy link
Contributor

@adamchalmers adamchalmers commented Oct 15, 2025

We've had a few suggestions that the light edges look weird in dark mode. Let's make the edges dark, even in dark mode.

Screenshot 2025-10-15 at 12 41 35 PM

Edges in sketch mode are still white:

Screenshot 2025-10-15 at 1 24 38 PM Screenshot 2025-10-15 at 1 25 09 PM

but unclosed edges drawn by the engine are black, and don't look good.

Screenshot 2025-10-15 at 1 30 30 PM

@adamchalmers adamchalmers requested review from a team as code owners October 15, 2025 18:19
@vercel
Copy link

vercel bot commented Oct 15, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
modeling-app Ready Ready Preview Comment Oct 15, 2025 7:12pm

Comment on lines 454 to 456
type: 'set_default_system_properties',
color: getThemeColorForEngine(opposingTheme),
color: getVolumeColorForEngine(opposingTheme),
Copy link
Contributor

Choose a reason for hiding this comment

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

There appears to be an inconsistency in the implementation. Line 462 uses getVolumeColorForEngine(opposingTheme) while lines 464-465 use getEdgeColorForEngine(opposingTheme). To properly implement dark edges as intended by this PR, line 462 should also use getEdgeColorForEngine instead of getVolumeColorForEngine.

Suggested change
type: 'set_default_system_properties',
color: getThemeColorForEngine(opposingTheme),
color: getVolumeColorForEngine(opposingTheme),
type: 'set_default_system_properties',
color: getEdgeColorForEngine(opposingTheme),

Spotted by Graphite Agent

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

@adamchalmers adamchalmers force-pushed the achalmers/darkmode-dark-edges branch from f31cd43 to f6e3a60 Compare October 15, 2025 18:33
@adamchalmers
Copy link
Contributor Author

@jacebrowning
Copy link
Contributor

@adamchalmers I'm going to put this back in draft since it's blocked. I think you can also revert that automated "Update snapshots" commit and merge in the latest main to fix the image conflict that was resolved in #8448.

@jacebrowning jacebrowning marked this pull request as draft October 16, 2025 17:33
@adamchalmers
Copy link
Contributor Author

Thanks Jace!

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.

2 participants