[#2092] Fixed terminal line height in the installer CLI video. #2093
Conversation
WalkthroughAdds configurable terminal styling (line-height and font-family) across docs rendering: new Node.js svg-term renderer, build-script swaps to that renderer, AsciinemaPlayer gains two props, tests added, and package dependency updated. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as Build script
participant Renderer as svg-term-render.js
participant FS as File system
rect rgba(200,230,255,0.6)
CI->>FS: ensure cast file exists
CI->>Renderer: node svg-term-render.js cast.json output.svg --line-height 1.1 [--at 1000]
Renderer->>Renderer: parse input, build theme (lineHeight,fontFamily)
Renderer->>Renderer: invoke svg-term to render SVG
Renderer-->>CI: SVG (+ status) / exit code
end
sequenceDiagram
participant MDX as installation.mdx
participant Comp as AsciinemaPlayer
participant Player as Asciinema Player lib
participant User
rect rgba(250,240,220,0.6)
MDX->>Comp: terminalLineHeight=1.0, terminalFontFamily="..."
Comp->>Comp: include props in options (create/update)
Comp->>Player: Player.create(options)
Player->>User: render terminal frames with configured styling
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2093 +/- ##
========================================
Coverage 68.04% 68.04%
========================================
Files 96 96
Lines 4731 4731
Branches 44 44
========================================
Hits 3219 3219
Misses 1512 1512 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AlexSkrypnyk
force-pushed
the
feature/2092-fix-term-line-height
branch
from
d5a1f85 to
47f91e2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
.vortex/docs/static/img/installer.pngis excluded by!**/*.png.vortex/docs/static/img/installer.svgis excluded by!**/*.svg.vortex/docs/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
.github/workflows/vortex-release-docs.yml(1 hunks).github/workflows/vortex-test-installer.yml(1 hunks).vortex/docs/.utils/svg-term-render.js(1 hunks).vortex/docs/.utils/update-installer-video.sh(2 hunks).vortex/docs/content/getting-started/installation.mdx(1 hunks).vortex/docs/package.json(1 hunks).vortex/docs/src/components/AsciinemaPlayer/AsciinemaPlayer.js(4 hunks).vortex/docs/tests/unit/AsciinemaPlayer/AsciinemaPlayer.test.js(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
.vortex/docs/src/components/AsciinemaPlayer/AsciinemaPlayer.js (1)
.vortex/docs/.utils/svg-term-render.js (1)
theme(60-84)
.vortex/docs/tests/unit/AsciinemaPlayer/AsciinemaPlayer.test.js (1)
.vortex/docs/src/components/AsciinemaPlayer/AsciinemaPlayer.js (1)
AsciinemaPlayer(3-104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-workflow (4)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-docs
🔇 Additional comments (11)
.vortex/docs/src/components/AsciinemaPlayer/AsciinemaPlayer.js (2)
12-13: LGTM! New props enhance terminal rendering control.The addition of
terminalLineHeightandterminalFontFamilyprops provides fine-grained control over terminal rendering appearance, directly addressing the line height issue mentioned in the PR objectives.
90-101: LGTM! Effect dependencies correctly updated.The effect dependency array now includes
terminalLineHeightandterminalFontFamily, ensuring the player reinitializes when these values change..vortex/docs/content/getting-started/installation.mdx (1)
35-35: LGTM! Documentation properly demonstrates the new prop.The
terminalLineHeight={1.0}prop is correctly applied to the AsciinemaPlayer component..github/workflows/vortex-release-docs.yml (1)
141-144: LGTM! Workflow correctly migrated to use project-local dependencies.The workflow now uses
yarn install --frozen-lockfileinstead of globalsvg-term-cliinstallation, aligning with the new Node-based rendering approach. The script path and working directory are correctly updated..vortex/docs/.utils/svg-term-render.js (2)
38-39: LGTM! Default values match AsciinemaPlayer component.The default values for
lineHeight(1.0) andfontFamilyare consistent with the AsciinemaPlayer component defaults, ensuring rendering consistency across documentation assets.
96-108: LGTM! Proper error handling and informative output.The script includes proper try-catch error handling and outputs useful information including lineHeight, fontFamily, and timestamp values for debugging.
.github/workflows/vortex-test-installer.yml (1)
63-66: LGTM! Test workflow aligned with release workflow.The changes mirror those in the release workflow, ensuring consistent installer video generation across both workflows.
.vortex/docs/.utils/update-installer-video.sh (2)
316-319: LGTM! Defensive early check for cast file existence.The early existence check for the cast file provides better error reporting before attempting SVG conversion.
321-321: The review comment references files that do not exist in this codebase.The verification found that:
AsciinemaPlayer.js(referenced at line 12) — not foundinstallation.mdx(referenced at line 35) — not foundupdate-installer-video.sh(referenced at line 321) — not foundThe only
line-heightreference in the codebase is inweb/modules/custom/ys_base/css/ys_base.css(line 99), which is unrelated to the SVG/terminal rendering concern raised in the review. The comment appears to reference code from a different repository or an outdated version.Likely an incorrect or invalid review comment.
.vortex/docs/tests/unit/AsciinemaPlayer/AsciinemaPlayer.test.js (2)
210-285: LGTM! Comprehensive test coverage for new props.The test suites thoroughly cover both
terminalLineHeightandterminalFontFamilyprops, including:
- Default values
- Custom values
- Multiple value variations
763-846: LGTM! Library integration tests verify proper prop propagation.These tests confirm that both
terminalLineHeightandterminalFontFamilyare correctly passed to the external AsciinemaPlayer library, including default value handling.
| "react-dom": "^18.0.0", | ||
| "svg-term-cli": "^2.1.1" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
Verify that svg-term-cli provides the required 'svg-term' module.
The new utility script at .vortex/docs/.utils/svg-term-render.js (line 18) imports svg-term using require('svg-term'). Please confirm that the svg-term-cli package provides this module, or if a separate svg-term dependency is needed.
🏁 Script executed:
#!/bin/bash
# Check if svg-term-cli provides the svg-term module
npm view svg-term-cliLength of output: 890
Add svg-term as an explicit dependency.
The [email protected] package includes svg-term: ^1.1.3 as a dependency, so the require('svg-term') call in .vortex/docs/.utils/svg-term-render.js will work. However, since the code directly requires svg-term (rather than svg-term-cli), it should be explicitly listed in package.json as a direct dependency rather than relying on a transitive dependency. Add "svg-term": "^1.1.3" to the dependencies in .vortex/docs/package.json.
🤖 Prompt for AI Agents
.vortex/docs/package.json around lines 40-41: the project requires svg-term
directly in .vortex/docs/.utils/svg-term-render.js but only depends on
svg-term-cli (which pulls svg-term transitively); add "svg-term": "^1.1.3" to
the dependencies object in .vortex/docs/package.json so svg-term is an explicit
direct dependency and will be installed reliably.
Closes #2092
Summary by CodeRabbit
New Features
Documentation
Tests
Chores