refactor: unify default strokeColor for outline shapes #1169
Conversation
This file contains 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
Codecov Report
@@ Coverage Diff @@
## main #1169 +/- ##
==========================================
- Coverage 64.77% 62.06% -2.72%
==========================================
Files 64 59 -5
Lines 7515 7112 -403
Branches 1760 1708 -52
==========================================
- Hits 4868 4414 -454
- Misses 2521 2615 +94
+ Partials 126 83 -43
|
± Registry diff📊 PerformanceKeyNote that each bar component rounds up to the nearest 100ms, so each full bar is an overestimate by up to 400ms. If a row has only one bar instead of four, that means it's not a trio and the bar just shows the total time spent for that example, again rounded up to the nearest 100ms. Data |
Deploying with
|
| Latest commit: |
0a2953a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f4510b97.penrose-72l.pages.dev |
| Branch Preview URL: | https://default-props.penrose-72l.pages.dev |
Right, this is because we just allocate We may want to come up with a more stable sampling strategy? But I'm not sure if that's possible or worth it. |
samestep
approved these changes
Dec 20, 2022
There was a problem hiding this comment.
Sorry for taking so long to review this! The code changes look great. For the changes to the generated diagrams/ we might want to update some variations though... would be nice if Penrose had a way to generate a grid of different variations to let you quickly pick one that looks good.
Gotta merge #1287! |
samestep
approved these changes
Jun 5, 2023
There was a problem hiding this comment.
Thanks for updating the registry diagram variations! I'm good to merge this, but just for future reference, I did want to note the few specific diagrams that still seem to have meaningful changes by this PR:
-
curve-examples/cubic-bezier.svg(looks upside down now?) -
geometric-queries/closest-point/test-group.svg(now the circle and the rectangle overlap) -
geometric-queries/closest-point/test.svg(now the polyline and the triangle overlap) -
geometry-domain/siggraph-teaser.svg(now the angle$qpr$ is a bit narrow) -
graph-domain/textbook/sec1/fig10.svg(the graph is planar but now not drawn so) -
graph-domain/textbook/sec1/fig12.svg(the labels RRP4 and RRP41 look ambiguous now) -
graph-domain/textbook/sec1/fig13.svg(the Team 2 and Team 5 labels look weird) -
graph-domain/textbook/sec1/fig8b.svg(a lot of the labels are now clumped) -
graph-domain/textbook/sec2/fig11c.svg(the graph is planar but no longer drawn so) -
walk-on-spheres/nested-estimator.svg(the spheres are now all clumped together on one side)
To clarify, I don't even necessarily think that it's worth changing the variations in this PR; I'm mostly just writing these down to note the sorts of ways that diagrams can qualitatively change when we update the variation. I think it's an interesting research problem to think about ways that we can make it more manageable to maintain our registry going forward under this sort of change.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Related issue/PR: #430
This PR makes
#000the defaultstrokeColorfor "outline" shapes such asLine,Path, andText.Pathis consider an outline shape out of uncertainty that the path might not be closed.Implementation strategy and design decisions
Examples with steps to reproduce them
See deployment
allShapes-allShapesexample.Checklist
diagrams/folderOpen questions
Looks like this impacted variations of many diagrams. Why? (cc: @samestep) Looks like even when the Style props remain the same, as long as there's a change to the defaults, the resulting diagrams will change.