FEATURE: Add site setting and wizard step to set base font (PR #10250)

This commit builds on Neil’s work, who added a site setting to control the base font, and adds a wizard step to set the base font. It looks like this:

image

GitHub

This is looking nice. I don’t see anything wrong with the code but I have a couple comments:

  • It looks like these fonts are awfully similar. Maybe my eyesight is bad but I can’t really tell the difference?
  • Are there issues with CSP for adding a style tag like this? cc @pmusaraj

No, I don’t think there are any CSP issues with inline style tags (only inline JS).

        description: "Choose a font that matches your theme or branding."

My thinking here is that we don’t tell them that other settings can be changed (they all can.) And themes seems a bit advanced at this point.

Is this still used?

This is used for the site setting.

Yes, I agree with you. I decided to remove this message completely and leave only the font previews, similar to how we do it on the themes page. :blush:

looks like we need a rebase @nbianca

This is adding 10s of megs to our git repo, perhaps we download these fonts on demand? I know this will complicate things but I do worry about adding lots of unused assets to our main repo.

Regarding inline style, I don’t think there are CSP concerns but I do much prefer keeping styling out of HTML if we can it is a lot harder to reason about things.

One thing we could possibly do here is just maintain a bunch theme components and have the UI simply glue up the theme component to switch the font, that would keep this extension super clean and we would not have to change core that much only wizard.

1 Like

@SamSaffron - @nbianca and I chatted about this. I’m worried that trying to automatically install/update/attach/detach theme components using the wizard will be quite risky. There are so many edge cases to think about. (e.g. what if the component is already installed on a non-default theme? What if it’s disabled? What if it’s not up to date? How/when do we update it? etc.)

Also, if we do this with remote theme components, we will have to scrap the live preview @nbianca added to the wizard (because we won’t have access to the font files).

I notice that the bulk of the file size comes from just two fonts (4mb each). We could scrap those options and save a lot of space.

Or, if we really wanted to keep these fonts out of the core repo, maybe we could package them in a Gem? (Kinda like how ember works at the moment)

1 Like

Can you publish that gem instead of using git directly?

Why do you need to put this style inline?

Did you mean to commit that file?

It’s a bit tight on the test side. Can you add an integration test that go through the wizard and ensure the font was properly changed?

Sure, I can do that.

we need to improve this, we can not do this every time we boot. we could do this as a custom step on our scss compiler or something like that. writing a scss file every time the app boots is very fragile.