Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions src/theme/__tests__/fonts.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,50 @@ describe('configureFonts', () => {
});
});

it('applies flat properties to every variant when the config also has per-variant entries', () => {
mockPlatform('ios');
const { configureFonts, typescale } = loadFonts();

const fonts = configureFonts({
config: {
fontFamily: 'NotoSans',
bodyLarge: {
fontSize: 18,
},
},
});

expect(fonts).toEqual({
...Object.fromEntries(
Object.entries(typescale).map(([variantName, variantProperties]) => [
variantName,
{ ...variantProperties, fontFamily: 'NotoSans' },
])
),
bodyLarge: {
...typescale.bodyLarge,
fontFamily: 'NotoSans',
fontSize: 18,
},
});
});

it('does not add flat properties of a mixed config as typescale variants', () => {
mockPlatform('ios');
const { configureFonts } = loadFonts();

const fonts = configureFonts({
config: {
fontFamily: 'NotoSans',
bodyLarge: {
fontSize: 18,
},
},
});

expect(fonts.fontFamily).toBeUndefined();
});

it('should be deterministic', () => {
mockPlatform('ios');
const { configureFonts } = loadFonts();
Expand Down
52 changes: 32 additions & 20 deletions src/theme/fonts.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,34 +13,41 @@ function configureFontsConfig(
return typescale;
}

const isFlatConfig = Object.values(config).every(
(value) => typeof value !== 'object'
);

if (isFlatConfig) {
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
return Object.fromEntries(
Object.entries(typescale).map(([variantName, variantProperties]) => [
variantName,
{ ...variantProperties, ...config },
])
) as Typescale;
// A config entry is either a whole variant (an object, e.g. `bodyLarge: { fontSize: 18 }`)
// or a single font property shared by every variant (e.g. `fontFamily: 'NotoSans'`).
// Both may appear in the same config, so they are collected separately instead of
// classifying the config as a whole.
const sharedProperties: Record<string, unknown> = {};
const variantOverrides: Record<string, object> = {};

for (const [key, value] of Object.entries(config)) {
if (typeof value === 'object' && value !== null) {
variantOverrides[key] = value;
} else {
sharedProperties[key] = value;
}
}

const typescaleByVariant: Partial<
Record<string, Typescale[keyof Typescale]>
> = typescale;

return Object.assign(
{},
typescale,
...Object.entries(config).map(([variantName, variantProperties]) => ({
[variantName]: {
const variantNames = new Set([
...Object.keys(typescale),
...Object.keys(variantOverrides),
]);

// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
return Object.fromEntries(
Array.from(variantNames, (variantName) => [
variantName,
{
...typescaleByVariant[variantName],
...variantProperties,
...sharedProperties,
...variantOverrides[variantName],
},
}))
);
])
) as Typescale;
}

export default function configureFonts(params?: {
Expand All @@ -51,6 +58,11 @@ export default function configureFonts(params?: {
config?: Partial<Record<TypescaleKey, Partial<TypescaleStyle>>>;
}): Typescale;
// eslint-disable-next-line no-redeclare
export default function configureFonts(params: {
config: Partial<TypescaleStyle> &
Partial<Record<TypescaleKey, Partial<TypescaleStyle>>>;
}): Typescale;
// eslint-disable-next-line no-redeclare
Comment on lines +61 to +65

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

wdyt about supporting custom variants here too?
custom variants are documented, but { fontFamily, customVariant: {...} } still produces TS error because this overload only accepts TypescaleKey

maybe we can use a generic mapped overload that treats TypescaleStyle keys as shared properties & other object keys as variants?

it might look smth like that:

Suggested change
export default function configureFonts(params: {
config: Partial<TypescaleStyle> &
Partial<Record<TypescaleKey, Partial<TypescaleStyle>>>;
}): Typescale;
// eslint-disable-next-line no-redeclare
type MixedFontsConfig<T extends Record<string, unknown>> = T & {
[K in keyof T]: K extends keyof TypescaleStyle
? TypescaleStyle[K]
: K extends TypescaleKey
? Partial<TypescaleStyle>
: TypescaleStyle;
};
type CustomFontVariants<T extends Record<string, unknown>> = {
[K in Exclude<
keyof T,
keyof TypescaleStyle | TypescaleKey
>]: TypescaleStyle;
};
export default function configureFonts<
T extends Record<string, unknown>,
>(params: {
config: MixedFontsConfig<T>;
}): Typescale & CustomFontVariants<T>;
// eslint-disable-next-line no-redeclare

btw, this could be handled separately in follow-up PR

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agreed, and I would rather keep this PR at its current size given it is already approved. I will open the follow-up once this lands.

Two things I checked in the meantime.

The gap is types-only. configureFontsConfig already builds custom variants at runtime: variantNames unions Object.keys(typescale) with Object.keys(variantOverrides) (src/theme/fonts.tsx:35-38), so { fontFamily, customVariant: {...} } produces the correct object today and it is only the overloads that reject it. Confirmed the error you describe: TS2769: No overload matches this call. The last overload gave the following error. Type 'string' is not assignable to type 'TypescaleStyle'.

Your sketch works. I prototyped it locally: T infers through MixedFontsConfig<T>, configureFonts({ config: { fontFamily, customVariant: {...} } }).customVariant.fontSize comes back as number, and the existing call shapes ({ fontFamily } alone, { fontFamily, bodyLarge }, and no argument) all still compile.

The one thing I want to settle in the follow-up is overload ordering. Placed first, the generic also claims the plain and mixed built-in cases, so their return type becomes Typescale & CustomFontVariants<T> instead of plain Typescale. That is structurally the same thing, but I would rather have a test per config shape before changing which overload wins.

export default function configureFonts(params: {
config: Record<string, TypescaleStyle>;
}): Typescale & { [key: string]: TypescaleStyle };
Expand Down