Repository navigation
Branding: screenshot alignment, border, and side-by-side step layout - #290
Conversation
A brand profile's new Screenshots card sets, for PDF, Word and HTML exports (and GitHub Pages): - Alignment: left, center or right for screenshots and flowcharts narrower than the page. - Border around screenshots, in a chosen color. - Step layout: stacked, or side by side with the screenshot on the left and the description on the right. Steps without a description stay stacked; HTML goes back to one column on narrow screens. Everything defaults to what exports did before. AI agents can read and set the options through the brand profile tools.
|
@codex review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d40dba788
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| parts.push(blocks(groups.beforeDescription), blocks(groups.beforeImage)); | ||
| parts.push(`<div class="step-split">${figure}${desc}</div>`); | ||
| parts.push(blocks(groups.afterDescription), blocks(groups.afterImage)); |
There was a problem hiding this comment.
When a step has a screenshot, a description, and a callout positioned after-description, enabling side-by-side layout moves that callout below the screenshot instead of above both columns. stepContentGroups() puts these callouts between the description and screenshot in stacked layout, so they fall under the documented rule that intervening callouts come first. The same ordering occurs in the PDF and Word side-by-side branches. Emit afterDescription before the split layout, alongside beforeImage, to preserve that rule and avoid placing instructions or warnings after the visual they precede.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4b7b81f. I didn't move them above both columns; callouts now keep their stacked-order neighbors in all three formats: one placed after the description sits under the description in the right column, and one placed before the image sits above the screenshot in the left column (callouts before the description stay above both columns, after the image below). New tests check the order in PDF, Word and HTML.
| if (align === 'center') rules.push('.shot img { margin-left: auto; margin-right: auto; }'); | ||
| if (align === 'right') rules.push('.shot img { margin-left: auto; margin-right: 0; }'); |
There was a problem hiding this comment.
For a flowchart containing a valid step link, chartLinks() wraps the image in .chart-links, whose existing CSS uses display: inline-block. That wrapper shrink-wraps the image and stays left-aligned in the figure; auto margins on the image inside it cannot move the wrapper. Consequently, Center and Right have no effect on linked flowcharts narrower than the page in either HTML export, despite the new alignment setting explicitly supporting flowcharts. Apply alignment to the wrapper while keeping the image and its link overlays together.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4b7b81f: Center and Right now also set text-align on .shot, which moves the inline-block .chart-links wrapper (image and link overlays together); plain images still use the auto margins. Checked in Electron with a linked flowchart: left 268–908, center 342–982, right 416–1056 inside a 268–1056 figure.
…charts Side by side moved a callout placed after the description below the screenshot. Callouts now stay where the stacked layout has them: one placed before the image sits above the screenshot in the left column, one placed after the description sits under it in the right column (PDF, Word, HTML). Center and Right didn't move a flowchart with step links in HTML: its inline-block link wrapper ignored the image's margins. The figure's text-align now moves the wrapper too.
Issue
Part of #286 (points 3–5: image alignment, screenshot border, side-by-side layout).
Summary
A brand profile gets a Screenshots card with three options for PDF, Word and HTML exports (and GitHub Pages, which uses the HTML):
Every option defaults to what exports already do, so existing profiles and unbranded exports don't change.
Changes
core/branding.js: the profile getsscreenshots: { align, border, borderColor, layout }, normalized with safe defaults.a:ln), so it can still be edited in Word..shot imgmargins..step-split, a CSS grid that goes back to one column under 720 px.screenshots(align,border,border_color,layout), with values validated.docs/BRANDING.mdhas a new Screenshots section.Area
Testing
bash tests/run_test.shpassesTested manually (describe below)
npm test: 1511 pass, 0 fail. The newtests/unit/branding-screenshots.test.jscovers:jc,a:ln, and the borderless two-column table.step-splitmarkup in simple and rich exportsnpm run translations: 1880 of 1880 in every language.Manual, WSL/Linux:
Not checked: Word itself (only LibreOffice), and Windows.
Screenshots or recordings
Settings → Branding → Edit brand profile → Screenshots
PDF, Center + border: before (left) and after (right). A tall screenshot is narrower than the page.
PDF, Side by side
Word, Center + border (rendered with LibreOffice)
Word, Side by side
HTML, Center + border and Side by side
Rollout notes
None. No migration: existing profiles read the new options as their defaults.
Conflicts with #288 (export translations + annotation fonts): both change
exporters/pdf.js,exporters/docx.js,exporters/html.js,app/renderer/branding.jsand the locale files. When resolving:t()/tc()fromexportText(ast)) and this PR's layout code.Checklist
t(), andnpm run translationslists nothing missing