feat: renderer adds tightest viewbox metadata to SVG
#1444
Merged
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
± 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 |
rjainrjain
changed the title
rjainrjain
changed the title
RenderStatic adds tightest viewbox metadata to SVG
rjainrjain
changed the title
RenderStatic adds tightest viewbox metadata to SVGrenderer adds tightest viewbox metadata to SVG
Deploying with
|
| Latest commit: |
cf67f70
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8cef8431.penrose-72l.pages.dev |
| Branch Preview URL: | https://rijul-crop.penrose-72l.pages.dev |
|
The PR was still a draft when @rjainrjain requested reviews. Marked it as ready just now :P. |
wodeni
approved these changes
Jun 5, 2023
There was a problem hiding this comment.
Great job on this PR @rjainrjain 💯! Very nice PR description and clean implementation. Looking forward to your future PRs :D. I had an optional suggestion below about null checking.
Also noting a weird thing about previews with imported SVGs. Don't have to solve this in this PR, but perhaps this has something to do with #1288.
| const cropped = svgDoc.querySelector("croppedViewBox")?.innerHTML; | ||
| const svgNode = svgDoc.querySelector("svg")!; | ||
| if (!(cropped === undefined)) { | ||
| svgNode.setAttribute("viewBox", cropped!); |
There was a problem hiding this comment.
Suggested change
| svgNode.setAttribute("viewBox", cropped!); | |
| svgNode.setAttribute("viewBox", cropped.innerHTML); |
This should typecheck without the assertions?
There was a problem hiding this comment.
hmm, I tried this out, along with the suggestion below — it still requires the assertion at some point because cropped is still possibly null when passing cropped.innerHTML to setAttribute.
| example.preview!, | ||
| "image/svg+xml" | ||
| ); | ||
| const cropped = svgDoc.querySelector("croppedViewBox")?.innerHTML; |
There was a problem hiding this comment.
Suggested change
| const cropped = svgDoc.querySelector("croppedViewBox")?.innerHTML; | |
| const cropped = svgDoc.querySelector("croppedViewBox"); |
See below for doing stuff after undefined check
|
Should've suggested just one change instead of 3 :(. My bad! Hopefully you can use the "Add suggestion to batch" feature, which is kinda new? |
Co-authored-by: Wode "Nimo" Ni <wn2155@columbia.edu>
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
Resolves #1261.
This PR takes the approach described in #1261 of providing a way of
by calculating the tightest cropped viewbox based on all shapes in the diagram and adding the viewbox values as metadata in the
<penrose>tag in the exported SVG. Replacing the viewbox values with these cropped values will solve the problem of sub-production level diagram previews.Implementation strategy and design decisions
Rather than modifying the generated SVG after the
rendereris through with it, as initially planned, we calculate the cropped viewbox values using the bounding boxes of theshapes: Shape<Num>generated inRenderStatic, adding the metadata then as well. In DiagramPanel.tsx we add a check to create a new metadata tag if there is none, or simply to add children to the one ostensibly created in theRenderStaticstep. In ExamplesBrowser.tsx we set the preview SVG's viewbox to the cropped values if the metadata is present, and display the SVG previews.Examples with steps to reproduce them
In the Penrose editor, click on the first example and inspect the SVG. It should have the following metadata.
<penrose><croppedViewBox>32.315104085276914 0.7731797378698957 601.3836522786437 601.3836522786437</croppedViewBox></penrose>After clicking the SVG download button in the editor, inspecting once more will show the following metadata.
Checklist
Open questions
Whether to provide an option in the editor to switch to cropped view.