imbajin commented on PR #418: URL: https://github.com/apache/incubator-hugegraph-doc/pull/418#issuecomment-3343427624
## PR Review Summary ### โ Overall Assessment The implementation for self-hosting fonts is technically sound and achieves the stated goals of improving privacy, performance, and reliability. The PR effectively removes external CDN dependencies. ### ๐ Review Findings **Critical Problems**: None identified **Should Fix** (2 items): 1. Font URL paths using absolute paths could break in subdirectory deployments 2. Commented code should be removed rather than disabled **Nice to Have** (3 items): 1. Unicode range subsetting for multilingual fonts to optimize performance 2. Font preloading strategy for critical fonts 3. Documentation improvements for future maintainers ### ๐งช Additional Testing Recommendations - Test font loading in subdirectory deployments (e.g., `/docs/` instead of root `/`) - Verify font rendering in browsers without WOFF2 support - Test offline mode to confirm fonts load without network access - Measure performance impact with Lighthouse before/after comparison ### โ Pre-merge Checklist - โ No breaking changes to public APIs - โ Reasonable PR size (~1.8MB font files is acceptable) - โ Clear rollback path available - โ Privacy improvement achieved - โ ๏ธ Consider documenting this change in release notes The privacy and performance benefits justify the added font files. Main concerns are around path handling for different deployment scenarios. Great work on implementing this improvement! ๐ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
