Repository navigation
fix(pptx): preserve picture source crops in svg exports - #88
Conversation
github-actions
Bot
added
documentation
commit: |
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⛔ Files ignored due to path filters: 1
⚙️ Run configuration
⛔ Files ignored due to path filters: 1
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check The changes address #87.SvgExporterapplieswrapSvgImageCropto exported image elements. The wrapper retains the destination frame and applies signed source insets through an SVG viewport. Tests …Out of Scope Changes check The changed files are limited to the shared PPTX SVG converter, its crop helper, and focused converter tests. The helper and tests directly support the crop-export fix in #87. No unrelated product are…Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly describes the main change: preserving picture source crops during SVG export. It is concise and specific.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Bundle size4 of 55 entry points changed size against main (3846a53):
All entry points
Each entry point with the files it imports statically, gzipped, before minification. Measured by |
ChristopherVR
left a comment
ChristopherVR
left a comment
There was a problem hiding this comment.
Thanks, this looks correct.
Wrapping the image in a nested <svg> whose viewBox selects the source rect keeps the destination frame, the outer transforms (rotation, flips) and the image-effect filters as they were, and signed insets give the expected padding. Uncropped pictures export byte-for-byte as before and invalid geometry falls back to the plain image. The CLI test on a saved deck is a nice touch.
Requests and notes:
- Please drop
docs/bug-repros/pptx-svg-picture-crop.png; screenshots are better kept in the PR description than committed underdocs/. - Nit: the wrapper interpolates its numbers directly, while the rest of
SvgExporterwrites attributes throughattrs(). Not a correctness issue. - Out of scope, for a follow-up: SVG export still ignores the stretch fill rect (
a:stretch/a:fillRect), which the on-screen renderer applies together with the crop.
b5e8b9a to
58dcb3b
Compare
github-actions
Bot
removed
the
documentation
|
Thanks for the review, @ChristopherVR! Updated in 58dcb3b: removed the PNG from the commit and moved it to GitHub attachments in the PR and issue descriptions. The |
2 participants
What does this change?
Fixes #87.
Apply picture source insets through a clipped SVG viewport so SVG exports retain the authored crop and destination frame. Preserve signed insets, image effects and outer transforms; leave uncropped images unchanged and fall back for invalid crop geometry.
Type of change
Area
src/core/pptx/converter: shared headless SVG export, including the CLI.Testing
bun run verify --base 260a953eabdd657fb744b8c4881d03f51da27a45: all 21 steps passed, including affected core/UI tests, builds, typechecks and viewer suites with React 18 compatibility.e2e:local-onlyexport regressions passed.b5e8b9a353f1ceef73a13c4f987c75b4e6c28d40withscope=changed.Screenshots / recordings
Reproduction PPTX
Summary by CodeRabbit