Skip to content

Conversation

@louwers
Copy link
Member

@louwers louwers commented Oct 31, 2025

Closes #1350

@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2025

Codecov Report

❌ Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.68%. Comparing base (4255ad2) to head (490050c).

Files with missing lines Patch % Lines
src/validate/validate_font_faces.ts 90.47% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1351      +/-   ##
==========================================
- Coverage   93.70%   93.68%   -0.02%     
==========================================
  Files         110      111       +1     
  Lines        4368     4389      +21     
  Branches     1386     1390       +4     
==========================================
+ Hits         4093     4112      +19     
- Misses        275      277       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@louwers louwers requested a review from HarelM October 31, 2025 12:04
@github-actions
Copy link
Contributor

github-actions bot commented Oct 31, 2025

Size Change: +537 B (+0.4%)

Total Size: 134 kB

Filename Size Change
dist/index.cjs 67.5 kB +267 B (+0.4%)
dist/index.mjs 66.5 kB +270 B (+0.41%)

compressed-size-action

@HarelM
Copy link
Collaborator

HarelM commented Oct 31, 2025

Added a few minor comments.

Copilot AI review requested due to automatic review settings November 6, 2025 14:29
@louwers louwers force-pushed the validation-font-faces branch from c223a3d to de4f27c Compare November 6, 2025 14:29
@louwers louwers enabled auto-merge (squash) November 6, 2025 14:30
@louwers louwers requested a review from HarelM November 6, 2025 14:30
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds validation support for the font-faces property in style specifications. The font-faces property allows specifying custom font files for rendering text with different formats (string URLs or arrays of font face objects with unicode ranges).

  • Introduces a new validateFontFaces function to validate the font-faces property structure
  • Updates the schema to change font-faces type from array with value: "fontFaces" to a single custom type fontFaces
  • Adds comprehensive integration tests covering valid and invalid font-faces configurations

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/validate/validate_font_faces.ts New validator function for font-faces property that validates object structure, string URLs, and arrays of font face objects
src/validate/validate.ts Registers the new validateFontFaces validator in the VALIDATORS mapping
src/reference/v8.json Updates font-faces schema from array type to custom fontFaces type
build/generate-docs.ts Adds fontfaces case to map to font-faces.md documentation
test/integration/style-spec/tests/*.json Adds 10 test files covering validation scenarios for invalid types (string, number, boolean, array) and valid structures (empty object, string URL, array of font faces)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +8 to +14

interface ValidateFontFacesOptions {
key: string;
value: unknown;
styleSpec: typeof v8;
style: StyleSpecification;
validateSpec: Function;
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the general Function type is not type-safe. Consider using a more specific function signature or importing the correct type from the codebase, similar to how other validators handle this parameter.

Suggested change
interface ValidateFontFacesOptions {
key: string;
value: unknown;
styleSpec: typeof v8;
style: StyleSpecification;
validateSpec: Function;
import type {ValidationError} from '../error/validation_error';
interface ValidateFontFacesOptions {
key: string;
value: unknown;
styleSpec: typeof v8;
style: StyleSpecification;
validateSpec: (options: { key: string; value: unknown; styleSpec: typeof v8; style: StyleSpecification; }) => ValidationError[];

Copilot uses AI. Check for mistakes.
}
};

for (const [i, fontFace] of (fontValue as any[]).entries()) {
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using any[] cast bypasses type safety. Since fontValueType === 'array' was already checked on line 46, consider using Array<unknown> instead of any[] for better type safety.

Suggested change
for (const [i, fontFace] of (fontValue as any[]).entries()) {
for (const [i, fontFace] of (fontValue as Array<unknown>).entries()) {

Copilot uses AI. Check for mistakes.
@louwers louwers merged commit e0b8a84 into main Nov 6, 2025
7 checks passed
@louwers louwers deleted the validation-font-faces branch November 6, 2025 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

font-faces should be an object but validator requires an array

4 participants