Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
There was a problem hiding this comment.
Pull request overview
This PR adds tooling to rasterize SVG block icons to JPG format for use in email templates and dashboards. The implementation includes a Node.js script that processes field block icons and generates 48x48 pixel retina-ready JPG images with white backgrounds.
Changes:
- Added
rasterize-icons.mjsscript to convert SVG icons to JPG format - Generated JPG files for all 19 field block icons
- Added
sharpdependency for image processing
Reviewed changes
Copilot reviewed 5 out of 25 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| projects/packages/forms/tools/rasterize-icons.mjs | New script that finds and converts SVG icons to JPG format with appropriate error handling |
| projects/packages/forms/src/blocks/field-*/icon@2x.jpg | Generated JPG files (19 total) for all field blocks |
| projects/packages/forms/package.json | Added sharp dependency and rasterize-icons npm script |
| pnpm-workspace.yaml | Configured to allow building sharp native dependencies |
| pnpm-lock.yaml | Updated with sharp and its platform-specific dependencies |
| projects/packages/forms/README.md | Documented the rasterization process and usage |
| projects/packages/forms/changelog/add-forms-field-icon-rasterizer | Added changelog entry following project conventions |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Code Coverage SummaryCannot generate coverage summary while tests are failing. 🤐 Please fix the tests, or re-run the Code coverage job if it was something being flaky. Full summary · PHP report · JS report Coverage check overridden by
Coverage tests to be added later
|
|
Added Triforce as a reviewer because we're touching build tooling here. |
For emails that can be rendered in dark mode, it would be better if they were with a transparent background. |
| const __dirname = dirname( fileURLToPath( import.meta.url ) ); | ||
| const formsRoot = join( __dirname, '..' ); | ||
| const blocksDir = join( formsRoot, 'src', 'blocks' ); | ||
| const outputDir = join( formsRoot, 'src', 'contact-form', 'images', 'field-icons' ); | ||
|
|
There was a problem hiding this comment.
Feels like these folder and file matching specifics should be handled by the webpack config, not the script.
There was a problem hiding this comment.
Might be the case, but it's also true it's not likely the directory structure will be changing. And if it does, it will be easy to change the script. The final location is also still waiting for the final decision on where the images should be for public access (we're defining this on the email template step, still in process).
| const __dirname = dirname( fileURLToPath( import.meta.url ) ); | ||
| const formsRoot = join( __dirname, '..' ); | ||
| const blocksDir = join( formsRoot, 'src', 'blocks' ); | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Step 1: Discover icon files | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| const ICON_EXTENSIONS = [ 'icon.jsx', 'icon.tsx', 'icon.js' ]; |
There was a problem hiding this comment.
Same comment as here https://github.com/Automattic/jetpack/pull/46979/changes#r2773196270 — I'd expect the file rules from Webpack config, not here.
There was a problem hiding this comment.
Attempting to address both comments on ef97b44
| } | ||
|
|
||
| // Replace currentColor with black (won't render in standalone SVG/email) | ||
| svg = svg.replace( /currentColor/g, '#000' ); |
There was a problem hiding this comment.
Wouldn't sites' frontend still need svg's with currentColor?
There was a problem hiding this comment.
Good point, yes, we need to refine a couple things but likely this was not needed.
There was a problem hiding this comment.
Moved it to the rasterizer script. The measure is a failsafe as well, most of our icons don't actually use a value for the color (not even currentColor), they rely on CSS for it. There are a couple exceptions but a single one using fixed #000, changing that to currentColor on a separate PR.
manzoorwanijk
left a comment
There was a problem hiding this comment.
This looks fine to me. Apart from inline comments, I wonder whether there should be a CI step to generate those icons to ensure that they are not stale as they are generated on demand.
| await sharp( svgFile, { density: 96 } ) | ||
| .resize( 48, 48 ) |
There was a problem hiding this comment.
I have used Sharp before, so I know a little bit about it. 😄
Sharp's density controls how SVG units map to pixels. With all icons being 24x24 viewBox and density 96, sharp rasterizes them at 24 × (96/72) = 32px, then upscales to 48px. This produces blurry output.
To get native 48px rendering (no upscale), the density should be 144 (since 24 × 144/72 = 48). With density: 144, you won't need .resize(48, 48).
| await sharp( svgFile, { density: 96 } ) | |
| .resize( 48, 48 ) | |
| await sharp( svgFile, { density: 144 } ) |
This may not sound a great improvement for images as this small, but for larger images it makes a considerable difference.
There was a problem hiding this comment.
TIL! Not at all a user of sharp, this setup is mostly Claudo magic, but good to know! 75978c2
| */ | ||
| function postProcessSvg( svg ) { | ||
| // Unwrap nested <svg> — take the inner one | ||
| const nestedMatch = svg.match( /^<svg[^>]*>\s*(<svg[\s\S]*<\/svg>)\s*<\/svg>$/ ); |
There was a problem hiding this comment.
This regex is too brittle/fragile for me. Maybe add comments about what we expect here?
Why not use a DOM parser (like htmlparser2, which is lightweight) instead of a regex? That would make transformation predictable and testable.
There was a problem hiding this comment.
Added a comment, since svg files are machine generated adding a dependency (even as light as htmlparser2) feels a bit too much? Let me know if you have a strong opinion on this, I can add the parser if it seems safer.
|
|
||
| // Ensure xmlns | ||
| if ( ! svg.includes( 'xmlns=' ) ) { | ||
| svg = svg.replace( '<svg ', '<svg xmlns="http://www.w3.org/2000/svg" ' ); |
There was a problem hiding this comment.
How does this work for avgs without attributes - <svg> or are we sure we always have attributes?
There was a problem hiding this comment.
The target is not generic, the svg files are specific to wordpress/icons and those custom we crafted. Maybe it needs more grooming but we're not aiming (nor planning to) this to be a solution that mangles anything thrown at, just our specific set of icons. These might get new ones, though it's not something we expect to change/add too often. I'll look into it, but most icons are from a controlled source.
| if ( ! svgTag.includes( 'viewBox=' ) ) { | ||
| throw new Error( 'SVG is missing a viewBox attribute; cannot infer width/height.' ); | ||
| } | ||
| svg = svg.replace( /viewBox=/, 'width="24" height="24" viewBox=' ); |
There was a problem hiding this comment.
Maybe detect the size from the viewBox?
| // --------------------------------------------------------------------------- | ||
|
|
||
| try { | ||
| unlinkSync( runnerPath ); |
There was a problem hiding this comment.
If Step 3, 4, or 5 fails, this clean-up never happens, right? Maybe wrap them in try/catch?
| const require_ = createRequire( import.meta.url ); | ||
| const bundlePath = join( formsRoot, 'dist', 'extract-icons-bundle.cjs' ); | ||
| const bundleExport = require_( bundlePath ); | ||
| const icons = bundleExport.default || bundleExport; |
There was a problem hiding this comment.
Maybe add a check to count the icons created to ensure that we didn't miss any?
There was a problem hiding this comment.
Maybe, but should we harcode that? Can you elaborate on this?
The idea that popped up when we discussed this concern was to forcibly add a white background on the template. The reason being we need to use raster images due to lack of SVG support on all clients and those won't adapt to light/dark schemes, even with transparency (black fills with transparent bg over a dark surface). That said, this PR is mostly to bring up all the concerns and constraints and come up with the best approach. For example, we need to test if this would work on all email clients: @media (prefers-color-scheme: dark) {
img {
filter: invert(1);
}
} |
| 1. **`extract-icons`** — Renders each React icon component to static SVG markup and writes `icon.svg` files. | ||
| 2. **`rasterize-icons`** — Converts each `icon.svg` to a 48x48 retina PNG (`icon@2x.png`) with a white background, optimized for minimal file size. | ||
|
|
||
| Run the full pipeline: | ||
|
|
||
| ```bash | ||
| pnpm generate-icons | ||
| ``` | ||
|
|
||
| Or run each step individually: | ||
|
|
||
| ```bash | ||
| pnpm extract-icons # React components → icon.svg | ||
| pnpm rasterize-icons # icon.svg → icon@2x.png | ||
| ``` |
There was a problem hiding this comment.
README documents rasterize output as icon@2x.png, but tools/rasterize-icons.mjs actually writes ${blockName}@2x.png (e.g., field-email@2x.png) into src/contact-form/images/field-icons/. Please update the filename examples/comments here so they match the script output to avoid confusion when running the pipeline.
Possibly? Not yet though. This was thought to be a manual run since we don't usually change the icons that much. But there are still some open questions we need to go through. Right now, I think a manual generation is enough. Whenever we're changing an icon (likely on js/jsx/tsx) what I'd like to have is a watch that would trigger the generation (only when those icon files change). This way, once we're done changing whatever icon, the diff will show both a .svg and a .png file to be committed. Good for follow up though. |
Adds a Node.js script that converts SVG block icons (src/blocks/field-*/icon.svg) into 48x48 JPG files with white background for use in email templates. The output is named icon@2x.jpg (2x retina for 24x24 display). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Adds sharp as a devDependency for the rasterize-icons script and approves its build scripts in pnpm-workspace.yaml. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
First batch of 48x48 retina JPG icons generated from SVG sources using the rasterize-icons script. These are used in form notification email templates. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Renders React icon components to standalone SVG files via webpack + ReactDOMServer. Handles all icon patterns: inline @wordpress/primitives, @wordpress/icons via <Icon> wrapper, and direct icon references. Mocks @wordpress/components to avoid jsdom — the Icon wrapper is bypassed by extracting the inner element from props. Post-processing fixes: nested <svg> unwrapping, currentColor replacement with #000 for email rendering, attribute normalization. Adds `pnpm generate-icons` to run the full pipeline (extract + rasterize). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Re-extracts icon.svg files using the new extract-icons script. - field-hidden: minor attribute reordering from @wordpress/icons source - field-slider: currentColor replaced with #000 for email template use Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Output PNGs to a centralized directory alongside the email template assets rather than scattering them across individual block directories. Updates README to fix lingering JPG→PNG reference. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Throw an explicit error if a rendered SVG is missing a viewBox attribute, since width/height injection depends on it. Without this guard the replacement silently fails and produces icons without dimensions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ould produce noticeably crisper output
…cons-runner.js file gets cleaned up even if webpack, bundle execution, or SVG writing fails
59eb271 to
a0cc5d8
Compare
| /** | ||
| * Extract SVG icons from React icon components. | ||
| * | ||
| * Discovers all icon.{jsx,tsx} files in src/blocks/field-* directories, |
There was a problem hiding this comment.
The header comment says the script discovers icon.{jsx,tsx} files, but iconPipelineConfig.iconFilenames also includes icon.js. Update the comment (or the config) so the supported extensions are accurately documented.
| * Discovers all icon.{jsx,tsx} files in src/blocks/field-* directories, | |
| * Discovers all icon.{js,jsx,tsx} files in src/blocks/field-* directories, |
Proposed changes:
Adds a complete pipeline to extract and rasterize block field icons for use in email templates.
Icon extraction (
pnpm extract-icons)Renders React icon components (
src/blocks/field-*/icon.{jsx,tsx}) to standalone SVG files via webpack + ReactDOMServer. Handles all icon patterns:@wordpress/primitives— SVG/Path/Circle/Line components (14 icons)@wordpress/iconsvia<Icon>wrapper — Bypasses the Icon wrapper by extracting the inner element from props (3 icons)@wordpress/iconsreference — Resolves the.srcproperty (1 icon)Post-processing normalizes the SVG output:
<svg>elements (fixes prior branch's rating icon bug)xmlns,width,height,aria-hidden,focusableattributesMocks
@wordpress/componentsduring extraction to avoid needing jsdom.Icon rasterization (
pnpm rasterize-icons)currentColorwith#000on the fly for standalone renderingicon.svgto a 48x48 retina PNG (field-*@2x.png) withwhite backgroundusing sharp. Output goes tosrc/contact-form/images/field-icons/for use by the email template renderer.Combined pipeline
pnpm generate-iconsruns both steps in sequence.Other information:
Jetpack product discussion
None
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Clean test (from scratch)
From
projects/packages/forms/:Verify individual steps
Visual verification
Open a few PNGs to confirm they render correctly:
src/contact-form/images/field-icons/field-email@2x.png— envelope iconsrc/contact-form/images/field-icons/field-rating@2x.png— three stars (no nested SVG artifacts)src/contact-form/images/field-icons/field-slider@2x.png— slider line with dot (black, not invisible)src/contact-form/images/field-icons/field-hidden@2x.png— crossed-out eye icon