Serve fonts using css-helper instead of javascript - #1130
Conversation
WalkthroughMoves Font Awesome CSS import out of JavaScript into SCSS, adds Font Awesome SCSS imports and a font-copying script with an npm script, updates README with an "Updating Fonts" section, and replaces a sign-in link with a Turbo-disabled form button. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CLI as yarn / npm
participant Script as bin/copy_fontawesome_fonts
participant NM as node_modules/@fortawesome/fontawesome-free/webfonts
participant Public as public/webfonts
Dev->>CLI: run `yarn update:font` / `npm run update:font`
CLI->>Script: execute `ruby bin/copy_fontawesome_fonts`
Script->>NM: check source exists
alt source exists
Script->>Public: ensure directory exists
Script->>NM: list matching files (*.woff2, *.woff, *.ttf, *.eot)
loop per file
Script->>Public: copy file
Script->>Script: log "copied <file>"
end
Script->>Script: log completion
else source missing
Script->>Script: abort and print guidance
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 4
🧹 Nitpick comments (2)
bin/copy_fontawesome_fonts (2)
17-17: Consider removing.svgfrom the font extensions list.SVG fonts are deprecated and no longer recommended for web use. Modern browsers have excellent support for WOFF2 and WOFF formats, which offer better compression and performance.
-font_exts = %w[.woff2 .woff .ttf .eot .svg] +font_exts = %w[.woff2 .woff .ttf .eot]
18-22: Consider optimizing to skip unchanged files.The current implementation copies all font files on every run. You could optimize this by checking timestamps and only copying when the source is newer than the destination.
Dir.entries(source).each do |file| next unless font_exts.include?(File.extname(file)) - FileUtils.cp(File.join(source, file), File.join(dest, file)) - puts "Copied #{file}" + src_file = File.join(source, file) + dest_file = File.join(dest, file) + + if !File.exist?(dest_file) || File.mtime(src_file) > File.mtime(dest_file) + FileUtils.cp(src_file, dest_file) + puts "Copied #{file}" + else + puts "Skipped #{file} (up to date)" + end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
app/assets/fonts/fa-brands-400.woff2is excluded by!**/*.woff2app/assets/fonts/fa-regular-400.woff2is excluded by!**/*.woff2app/assets/fonts/fa-solid-900.woff2is excluded by!**/*.woff2app/assets/fonts/fa-v4compatibility.woff2is excluded by!**/*.woff2
📒 Files selected for processing (6)
README.md(1 hunks)app/assets/stylesheets/application.scss(1 hunks)app/assets/stylesheets/fontawesome.scss(1 hunks)app/javascript/application.js(0 hunks)bin/copy_fontawesome_fonts(1 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (1)
- app/javascript/application.js
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~79-~79: Ensure spelling is correct
Context: ... need to be updated, they can be copied form the nodemodules folder to assests:fonts...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~79-~79: Ensure spelling is correct
Context: ...e copied form the nodemodules folder to assests:fonts with the following command. yarn:p...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
README.md
77-77: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
81-81: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
🔇 Additional comments (1)
app/assets/stylesheets/application.scss (1)
1-1: LGTM! Good import ordering.The FontAwesome import is correctly placed before Bootstrap, ensuring fonts are loaded early in the cascade.
| "build:css": "sass ./app/assets/stylesheets/application.scss:./app/assets/builds/application.css --no-source-map --load-path=node_modules", | ||
| "postinstall": "ruby bin/copy_fontawesome_fonts" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that the copy script is executable and the path is correct.
The postinstall script references bin/copy_fontawesome_fonts. Ensure this file has executable permissions and exists at the specified path.
🏁 Script executed:
#!/bin/bash
# Verify the font copy script exists and is executable
# Check if the file exists
if [ -f "bin/copy_fontawesome_fonts" ]; then
echo "✓ File exists: bin/copy_fontawesome_fonts"
# Check if it's executable
if [ -x "bin/copy_fontawesome_fonts" ]; then
echo "✓ File is executable"
else
echo "✗ File is NOT executable - run: chmod +x bin/copy_fontawesome_fonts"
fi
# Check shebang
head -n 1 bin/copy_fontawesome_fonts
else
echo "✗ File not found: bin/copy_fontawesome_fonts"
fiLength of output: 129
Add executable permissions to the font copy script.
The file bin/copy_fontawesome_fonts exists but lacks executable permissions. Run chmod +x bin/copy_fontawesome_fonts to fix this.
🤖 Prompt for AI Agents
In package.json around lines 10 to 11, the postinstall script refers to
bin/copy_fontawesome_fonts which currently lacks executable permissions; make
the script executable by running chmod +x bin/copy_fontawesome_fonts locally,
verify it has the executable bit (e.g., ls -l), commit the change so the
executable mode is preserved in git (git add bin/copy_fontawesome_fonts && git
commit -m "Make copy_fontawesome_fonts executable"), and push the commit so
postinstall can run successfully.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
bin/copy_fontawesome_fonts (2)
17-17: Consider removing deprecated SVG font extension.SVG fonts are deprecated and rarely needed for modern web browsers, which support WOFF2 and WOFF formats.
Apply this diff if SVG fonts are not required:
-font_exts = %w[.woff2 .woff .ttf .eot .svg] +font_exts = %w[.woff2 .woff .ttf .eot]
18-22: Consider using Dir.glob for cleaner filtering.Using
Dir.globwith a pattern is more idiomatic Ruby thanDir.entrieswith manual filtering.Apply this diff to refactor using Dir.glob:
-font_exts = %w[.woff2 .woff .ttf .eot .svg] -Dir.entries(source).each do |file| - next unless font_exts.include?(File.extname(file)) - FileUtils.cp(File.join(source, file), File.join(dest, file)) +Dir.glob(File.join(source, '*.{woff2,woff,ttf,eot,svg}')).each do |file_path| + file = File.basename(file_path) + FileUtils.cp(file_path, File.join(dest, file)) puts "Copied #{file}" end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
public/webfonts/fa-brands-400.woff2is excluded by!**/*.woff2public/webfonts/fa-regular-400.woff2is excluded by!**/*.woff2public/webfonts/fa-solid-900.woff2is excluded by!**/*.woff2public/webfonts/fa-v4compatibility.woff2is excluded by!**/*.woff2
📒 Files selected for processing (2)
app/assets/stylesheets/application.scss(2 hunks)bin/copy_fontawesome_fonts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/assets/stylesheets/application.scss
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
README.md (3)
75-77: Add language specifier to code block.The fenced code block should specify
bashas the language for proper syntax highlighting and to satisfy linting rules.Apply this diff:
-``` +```bash bundle exec rails db:migrate:archiveBased on static analysis tools, this addresses the MD040 warning. --- `79-79`: **Remove extra space in heading.** The heading has two spaces after `##` instead of one, which is inconsistent with markdown conventions and other headings in this file. Apply this diff: ```diff -## Updating Fonts +## Updating FontsBased on static analysis tools, this addresses the MD019 warning.
83-85: Add language specifier to code block.The fenced code block should specify
bashas the language for proper syntax highlighting and to satisfy linting rules.Apply this diff:
-``` +```bash yarn update:fontBased on static analysis tools, this addresses the MD040 warning. </blockquote></details> <details> <summary>bin/copy_fontawesome_fonts (1)</summary><blockquote> `16-24`: **Consider optional improvements for robustness.** The file copying logic is correct and functional. A few optional enhancements could improve maintainability and user feedback: 1. Define `font_exts` as a constant for better maintainability 2. Track the count of copied files and warn if zero files were copied 3. Consider whether stale font files should be cleaned up (though this is likely not needed in practice) Example enhancement for counting: ```diff # Copy all font files font_exts = %w[.woff2 .woff .ttf .eot] +copied_count = 0 Dir.entries(source).each do |file| next unless font_exts.include?(File.extname(file)) FileUtils.cp(File.join(source, file), File.join(dest, file)) puts "Copied #{file}" + copied_count += 1 end -puts "All FontAwesome fonts copied to #{dest}" +puts "Copied #{copied_count} FontAwesome font(s) to #{dest}" +warn "Warning: No font files were copied!" if copied_count.zero?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)bin/copy_fontawesome_fonts(1 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
75-75: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
79-79: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
79-79: Multiple spaces after hash on atx style heading
(MD019, no-multiple-space-atx)
83-83: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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: Build
🔇 Additional comments (3)
bin/copy_fontawesome_fonts (3)
1-3: LGTM!The shebang and descriptive comment are appropriate. The comment accurately reflects the script's purpose and destination path.
6-8: LGTM!The path construction using
File.expand_pathwith__dir__is correct and ensures the script works regardless of the current working directory.
10-14: LGTM!The source directory validation with a helpful error message and the safe destination directory creation are well-implemented.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## staging #1130 +/- ##
========================================
Coverage 75.27% 75.27%
========================================
Files 50 50
Lines 1076 1076
========================================
Hits 810 810
Misses 266 266 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(2 hunks)app/assets/stylesheets/application.scss(2 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
79-79: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
⏰ 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: Build
🔇 Additional comments (4)
app/assets/stylesheets/application.scss (1)
1-6: Font Awesome integration looks good.The $fa-font-path variable is correctly set to align with the font copy workflow, and the SCSS imports follow Font Awesome best practices.
README.md (3)
61-70: Improved code block formatting.The Testing & Linting sections now properly use fenced code blocks, improving markdown consistency and readability.
75-77: Migration section properly formatted.The command is correctly wrapped in a fenced code block and is syntactically correct.
79-85: Fonts documentation section is complete and correct.The new section contains accurate documentation with proper command syntax, correct file paths, and consistent formatting with other sections in the file. The command aligns with the
update:fontnpm script added to package.json.
| .btn-login { | ||
| width: 100% | ||
| } |
There was a problem hiding this comment.
Add missing semicolon in CSS rule.
Line 85 is missing a semicolon after the width property, which is a critical syntax error that will cause CSS parsing to fail.
.btn-login {
- width: 100%
+ width: 100%;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .btn-login { | |
| width: 100% | |
| } | |
| .btn-login { | |
| width: 100%; | |
| } |
🤖 Prompt for AI Agents
In app/assets/stylesheets/application.scss around lines 84 to 86, the .btn-login
rule is missing a semicolon after the width property; open that file and add a
trailing semicolon so the rule reads width: 100%; then save and re-run your CSS
linter/build to ensure parsing succeeds.
1 participant
Checklist
origin/staging(git checkout staging ; git pull ; bundle exec rails db:reset ; git checkout BRANCH ; bundle exec rails db:migrate).Summary
Shortly summarize the changes in this pull request. Does it concern changes in the UI, add some screenshots. Are there related issues solved? Please, mention them (with 'fixes #xyz', see https://github.com/blog/1506-closing-issues-via-pull-requests), so they can be resolved automatically when merging this pull request.
Other information
If there is some other relevant and important information for this pull request, mention it here. For example, related pull requests or newly introduced conventions, packages or other dependencies.
Summary by CodeRabbit
New Features
Documentation
Style
Behavior
✏️ Tip: You can customize this high-level summary in your review settings.