Conversation
add dark palette
WalkthroughAdds a configurable Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
xsl/vitals.xsl (1)
13-13: Palette parameter plumbing into the root HTML class looks correct; consider adding an XSL default.The new
paletteparam andclass="palette-..."wiring are consistent with the rest of the PR. To make the transform more robust when run outsideentry.sh, you could give the parameter an XSL-level default:- <xsl:param name="palette" as="xs:string"/> + <xsl:param name="palette" as="xs:string" select="'classic'"/>This keeps current behavior but avoids an empty
class="palette-"if the param isn’t provided.Also applies to: 119-121
entry.sh (1)
173-177: Palette defaulting and Saxon parameter wiring look good.Reading
INPUT_PALETTE, falling back to"classic", and passingpalette=${palette}into Saxon is consistent with the action input and XSLT parameter. I don’t see any functional issues here.If you want slightly clearer logs, you could also echo the explicitly set palette (not only the default case), but that’s purely optional.
Also applies to: 225-225
sass/main.scss (1)
2-2: Palette-specific styles are wired correctly; consider contrast tweaks for the mild theme.The
.palette-dark/.palette-mildblocks line up with theclass="palette-{palette}"on<html>, and the stylelint disable/enable around them is scoped cleanly.One thing to revisit: in
.palette-mildyou setbackground-color: #777with text#333onbody, which is a relatively low contrast combination. It may be worth lightening the background or darkening the text there (and checking headings) to keep the “mild” look while staying comfortably readable.Also applies to: 83-144
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(1 hunks)action.yml(1 hunks)entry.sh(2 hunks)sass/main.scss(2 hunks)xsl/vitals.xsl(2 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~39-~39: Ensure spelling is correct
Context: ...HTML page; * palette (optional) color polette of the vitals HTML page; (default: clas...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ 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). (1)
- GitHub Check: make
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
entries/vitals-has-palette-option.sh (2)
13-24: Consider improving log file handling and reducing code duplication.The test script contains four nearly identical test blocks. While the repetition is clear and explicit, it could be refactored into a helper function to reduce maintenance burden and make future changes easier.
Additionally, each call to
tee log.txtoverwrites the log file, losing the output of previous test runs. If one of the later tests fails, you can only see the log from that final failed run, not the successful intermediate tests. Consider appending logs or using a timestamp-based naming scheme.Example refactoring (helper function):
run_palette_test() { local palette_name=$1 env "GITHUB_WORKSPACE=$(pwd)" \ 'GITHUB_REPOSITORY=foo/bar' \ 'GITHUB_REPOSITORY_OWNER=foo' \ 'INPUT_FACTBASE=test.fb' \ 'INPUT_OUTPUT=output' \ 'INPUT_VERBOSE=false' \ "INPUT_PALETTE=${palette_name}" \ 'INPUT_ADLESS=false' \ 'INPUT_GITHUB-TOKEN=THETOKEN' \ "${SELF}/entry.sh" 2>&1 | tee "log-${palette_name}.txt" grep "<html class=\"palette-${palette_name}\">" 'output/test-vitals.html' } # Test explicit palette values run_palette_test "dark" run_palette_test "mild" run_palette_test "classic" # Test default (no explicit INPUT_PALETTE) env "GITHUB_WORKSPACE=$(pwd)" \ 'GITHUB_REPOSITORY=foo/bar' \ 'GITHUB_REPOSITORY_OWNER=foo' \ 'INPUT_FACTBASE=test.fb' \ 'INPUT_OUTPUT=output' \ 'INPUT_VERBOSE=false' \ 'INPUT_ADLESS=false' \ 'INPUT_GITHUB-TOKEN=THETOKEN' \ "${SELF}/entry.sh" 2>&1 | tee log-default.txt grep '<html class="palette-classic">' 'output/test-vitals.html'This consolidation maintains the same test logic while making it easier to add new palettes and preserving logs from all test runs.
Also applies to: 26-37, 39-50, 52-62
24-24: Grep patterns could be more flexible.The current grep patterns match the exact HTML format:
<html class="palette-dark">. This works but is brittle—if attribute order changes or whitespace differs, the pattern breaks even if the feature works correctly.Consider a more flexible grep pattern:
# Instead of: grep '<html class="palette-dark">' 'output/test-vitals.html' # Use: grep -o 'class="palette-dark"' 'output/test-vitals.html' > /dev/nullor with regex to be more explicit:
grep 'class="palette-dark"' 'output/test-vitals.html' > /dev/nullThis allows for variations in attribute order or spacing while still validating the core requirement (the palette class is present).
Also applies to: 37-37, 50-50, 62-62
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)action.yml(1 hunks)entries/vitals-has-palette-option.sh(1 hunks)entry.sh(2 hunks)sass/main.scss(2 hunks)xsl/vitals.xsl(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- xsl/vitals.xsl
- sass/main.scss
- README.md
- entry.sh
🔇 Additional comments (2)
action.yml (1)
26-28: Palette input is well-documented.The description correctly lists all three supported palettes (
classic,dark,mild), addressing the prior feedback. The default value and format are appropriate.entries/vitals-has-palette-option.sh (1)
1-5: Shell script error handling is correct.The use of
set -e -o pipefailensures the test fails fast if any command fails, which is appropriate for test scripts.
|
@yegor256 review please |
|
@rultor merge |
3 participants
Close #31
Changes:
paletteoption was added. Possible values:classic,dark,mild.classicpalette is default.<html>tag.For example:
<html class="palette-dark">will use dark palette.darkandmildthey have to be adjusted and improved taking into account Integrate palette.css and remove redundant styles #461 Puzzles were added about this.darkandmildpalettes:Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.