Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: allow admins to configure font in the survey editor #27292

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

feat: allow admins to configure font in the survey editor #27292

wants to merge 5 commits into from
+78 −15

Conversation

lucasheriques
Copy link
Contributor

@lucasheriques lucasheriques commented Jan 6, 2025 •
edited
Loading

Problem

#25338

Changes

CleanShot.2025-01-06.at.18.11.44.mp4

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Does this work well for both Cloud and self-hosted?

How did you test this code?

@lucasheriques lucasheriques changed the title feat: allow custom fonts on surveys feat: allow admins to configure font in the survey editor Jan 6, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 6, 2025 •
edited
Loading

Size Change: 0 B

Total Size: 9.44 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 9.44 MB

compressed-size-action

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

64 snapshot changes in total. 0 added, 64 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@lucasheriques lucasheriques force-pushed the feat/allow-custom-fonts-on-surveys branch from 4212e3f to 6c1c9f8 Compare January 6, 2025 21:30
lucasheriques
lucasheriques commented Jan 6, 2025
View reviewed changes
frontend/src/scenes/surveys/SurveyCustomization.tsx
Comment on lines -144 to +147
}
onChange={(placeholder) => onAppearanceChange({ ...appearance, placeholder })}
disabled={!surveysStylingAvailable}
/>
</LemonField.Pure>
</>
<LemonField.Pure className="mt-2" label="Placeholder text">
<LemonInput
value={
appearance?.placeholder !== undefined
? appearance.placeholder
: defaultSurveyAppearance.placeholder
}
onChange={(placeholder) => onAppearanceChange({ ...appearance, placeholder })}
disabled={!surveysStylingAvailable}
/>
</LemonField.Pure>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changes here are just to remove the unneeded fragment

@corywatilo
Copy link
Contributor

Exciting! A few pieces of feedback:

  1. As a front end-oriented person, I'd be pretty disappointed to see only this list of fonts since this triggers my PTSD of being a front end developer back when the web was actually limited to these fonts. (I get this is an MVP and we're going to add more - woo! But just to say I hope we don't launch with just these.) =]

    • Even if we're going to do email surveys, I'd still opt for progressive enhancement and use good fonts on the web and just fall back to whatever we have to in email. It's also worth noting that recent versions of Mac OS come with a tonnn more fonts (eg: Proxima Nova), so I'd probably look at that list as a starting point.

  2. Mac OS has started shipping with a much larger library of fonts like Avenir Next, Baskerville, Noto Sans, Proxima Nova. It'd be worth including options for these (again, progressive enhancement), though denoting fallbacks might be used on other devices.

  3. Figma imports all Google Fonts and lets you pick any of them from there. Since that library has become a pretty solid resource of cross-compatible fonts, it's worth considering doing something similar. Some random implementation thoughts which may or may not be useful:

    • If we're planning on showing a preview of each font in the select, it probably makes sense to only load the single weight used in the select until a font is chosen
    • When loading a Google font on a customer's site, we can probably make a gut call to just import two weights (like 400 and 700) and omit italic (and let the OS infer it if necessary).
    • If the customer is already importing a Google font on their site, we might want an option to exclude us loading it on their site so we're not loading it twice! (Assuming we're not shadow dom'd and have access to the parent?)

  4. Obviously supporting custom fonts would be awesome, but that also introduces a bunch of variables:
    a. Can we just ask for their font string and assume they'll have the font loaded on their site for us to use?
    b. (We're not running in shadow DOM are we?) Do we host them upload fonts to us? Or if we reference their hosted font, any CORS issues?

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

64 snapshot changes in total. 0 added, 64 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lucasheriques @posthog-bot @corywatilo