Repository navigation
Conversation
30ba3b9 to
547008f
Compare
orkhanahmadov
left a comment
There was a problem hiding this comment.
Thanks for this, Zulfiqar. Listing the font that's actually in use is the right fix, and withCurrentFont is a good place for it. I'd like a few changes before merging, mostly so all the font pickers behave the same way.
1. Template Settings needs the same fix. It's the most common place an unlisted font ends up: the agent skill writes a stack into settings.fontFamily (every example in skills/templatical/reference/examples/ does, e.g. "Georgia, serif"), import-html sets it to the body font's first family ("Helvetica Neue"), and import-easy-email-pro passes the raw stack through. displayedFontFamily then falls back to fontsManager.defaultFont. Without config.fonts.defaultFont that's "Arial, sans-serif", which matches no option, so the select shows blank. With defaultFont set it shows the default while the email renders something else. The "custom font when disabled" reason in that comment no longer applies: customFontsEnabled / setCustomFontsEnabled were removed from useFonts(). Using withCurrentFont there and keeping the default fallback only for an empty value would make all three pickers consistent.
2. Equivalent stacks show up as a second option. See the inline comment on withCurrentFont.ts.
3. Docs. apps/docs/guide/fonts.md (and de/guide/fonts.md) says restricting builtIns "only affects the picker". With this change, a block using an excluded family shows that family in its picker. That's the honest behaviour, but brand-kit integrators will notice, so the guide should say so in both locales.
Smaller things are inline. Non-blocking: this is now the eighth copy of first-family parsing (each importer has one, plus useFonts.ts:150), and beefree/unlayer strip quotes differently from the rest. A shared helper in @templatical/types can be a follow-up, not this PR.
I applied this on current main: the 3 new test files pass and the editor typecheck is clean. CI hasn't run on the fork yet, and since this touches editor UI, the e2e suite should run too.
| const label = fonts.some( | ||
| (font) => font.label.toLowerCase() === firstFamily.toLowerCase(), | ||
| ) | ||
| ? current | ||
| : firstFamily; | ||
| return [{ value: current, label }, ...fonts]; |
There was a problem hiding this comment.
When the first family is already offered, this adds the full stack as a second option instead of reusing the one that's there. "Georgia, serif" lists "Georgia, serif" above "Georgia", and the same happens for "Arial, sans-serif", "'Times New Roman', serif" (byte-identical to BUILT_IN_FONT_STACKS) and "arial". The renderer resolves "Georgia" to "Georgia, serif" and matches case-insensitively, so both options give the same email, but the author can't tell that. These are exactly the stacks the skill examples write.
Suggest reusing the matching option: give it current as its value and keep its label and position. It stays inside this helper, the five toolbars' :value bindings don't change, and a new leading option is only added when the family really isn't offered. The "labels the stack in full" test would need updating to match.
| const label = fonts.some( | |
| (font) => font.label.toLowerCase() === firstFamily.toLowerCase(), | |
| ) | |
| ? current | |
| : firstFamily; | |
| return [{ value: current, label }, ...fonts]; | |
| const matchIndex = fonts.findIndex( | |
| (font) => font.label.toLowerCase() === firstFamily.toLowerCase(), | |
| ); | |
| if (matchIndex !== -1) { | |
| return fonts.map((font, index) => | |
| index === matchIndex ? { ...font, value: current } : font, | |
| ); | |
| } | |
| return [{ value: current, label: firstFamily }, ...fonts]; |
| if (!current || fonts.some((font) => font.value === current)) { | ||
| return fonts; | ||
| } | ||
| const firstFamily = current |
There was a problem hiding this comment.
TipTap keeps the raw inline value (FontFamily.parseHTML reads the style attribute via getStyleProperty), so pasted or imported spans come through here verbatim: inherit gets listed as a font named "inherit" (in the block toolbars that duplicates "Inherit font"), Arial, sans-serif !important is labelled with the !important, and a whitespace-only value gives an empty label that looks like the placeholder. Treating CSS-wide keywords and blank values as unset, and stripping !important before building the label, covers these.
| :model-value="textStyleAttr('fontFamily')" | ||
| :options="fontFamilies" | ||
| :options=" | ||
| withCurrentFont(fontFamilies, textStyleAttr('fontFamily')) |
There was a problem hiding this comment.
Font size, line height and letter spacing in this toolbar have the same problem. import-html writes font-size: Npx for any size other than 16, and FONT_SIZE_OPTIONS has no 13px or 15px, so 15px text reads "Default size". Have you considered handling an unlisted modelValue inside ToolbarSelect instead, with an optional label-formatter prop for fonts? That fixes all four selects at once, and this call site goes back to plain fontFamilies. Fine as a follow-up if you'd rather keep this PR to fonts.
| "@templatical/editor": patch | ||
| --- | ||
|
|
||
| The font pickers show the font in use when it isn't one of the listed fonts. A title, button, menu, table or countdown whose `fontFamily` is a full stack, such as `Helvetica Neue, Helvetica, Arial, sans-serif` from imported content, matched no option, so the select showed blank. The block's own font is now listed first, labelled by its first family, or in full when that family is already in the list, so `Arial, sans-serif` doesn't show as a second "Arial". The paragraph toolbar's font select does the same for the selected text, which showed "Default font" for an imported stack. |
There was a problem hiding this comment.
The first-party importers already cut stacks down to the first family (parseFontFamily takes split(",")[0]), so imports mostly produce a single unlisted family like Helvetica Neue, not a full stack. Could the note describe the real triggers: a family the picker doesn't offer (imports, builtIns exclusions), or a stack from API- or agent-written JSON? If Template Settings gets the fix, mention it here too.
|
|
||
| describe("ParagraphToolbar font select", () => { | ||
| it("passes the selection's font through withCurrentFont", () => { | ||
| expect(SRC).toMatch( |
There was a problem hiding this comment.
Source-level tests for ParagraphToolbar are an existing pattern, so this is fine as a guard. It does pin the code's shape rather than the behaviour, though. Could you add one Playwright spec? Seed a title with Helvetica Neue, Arial and a paragraph span with an unlisted stack, assert the selected option text in both pickers, then change the font and assert the extra option is gone.
| ]), | ||
| loadCustomFonts: async () => {}, | ||
| cleanupFontLinks: () => {}, | ||
| setCustomFontsEnabled: () => {}, |
There was a problem hiding this comment.
Nit: setCustomFontsEnabled isn't part of UseFontsReturn anymore. It's inherited from older mocks; you can drop it.
The font options are single family names, so a block or text selection whose font is a full stack (imported content often carries "Helvetica Neue, Helvetica, Arial, sans-serif") matched no option. The block toolbars' select went blank and the paragraph toolbar's read "Default font", hiding the font in use and dropping it at the next touch. withCurrentFont lists the font in use first, labelled by its first family, or in full when that would read the same as a listed font. Toolbar.vue uses it for the title, button, menu, table and countdown toolbars, and ParagraphToolbar.vue for the selected text, in the template because TipTap's attributes aren't reactive. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… Template Settings
A stack whose first family is offered ("Georgia, serif") takes over that
option instead of adding a second one. CSS-wide keywords and blank values
count as no font, and !important stays out of the label.
Template Settings lists the font in use the same way, and falls back to the
default font only when none is set, so it no longer shows the default while
the email renders another font.
The fonts guide says a block keeps showing a font builtIns excludes. A
Playwright spec covers the title and paragraph pickers.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
547008f to
9bdc0c7
Compare
Summary
If a block's font isn't one of the listed options, the font select shows nothing. Imported content often uses a full font stack like
Helvetica Neue, Helvetica, Arial, sans-serif, which never matches. You can't see which font is applied, and touching the select replaces it.This lists the font in use first, labelled by its first family ("Helvetica Neue"). If that family is already in the list, the full stack is shown instead, so you never get two identical "Arial" options.
It covers the block toolbars (title, button, menu, table, countdown) and the paragraph toolbar's font select.
Linked issues
n/a
Type of change
Affected packages
@templatical/editorChecklist
pnpm run cipasses locally (lint + typecheck + build + test)pnpm run test:e2epasses (only if this PR touches editor UI)pnpm exec changeset)Notes for reviewers
withCurrentFont, used by both toolbars.pnpm run cior e2e.🤖 Generated with Claude Code