-
Notifications
You must be signed in to change notification settings - Fork 6
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.
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
[#2030] Add option to upload and use custom fonts #963
Conversation
8d0ce83
to
bcb1512
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #963 +/- ##
===========================================
+ Coverage 94.74% 94.79% +0.04%
===========================================
Files 872 876 +4
Lines 30546 30657 +111
===========================================
+ Hits 28940 29060 +120
+ Misses 1606 1597 -9 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a final review yet - these are some comments for deeper research.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1df2bb7
to
e0ead12
Compare
The title on the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool feature. As this is overwriting the same file/URL I foresee caching issues, where someone uploads new fonts and then nobody sees them because browser cache.
If that happens we might have to fix cache headers (like etag+revalidate instead of just age)
e0ead12
to
8edfb29
Compare
"text_body_font", | ||
open_inwoner.configurations.models.CustomFontField( | ||
blank=True, | ||
help_text="Regular font for the text body. Supported are files with the .ttf extension.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor detail and not disapproved, but the message here may need to be more in 'active' language, something like "Upload body text font here. TTF font types only" - same as the one for the Headings.
Also I am wondering if we want this font upload to be all the way in the Admin bottom, instead of higher up, closer to the 'custom colors' section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the messages, but moving the admin inline seems to require modifications to the admin template. Let's discuss and perhaps create a separate task for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pi-sigma to add to that: the amount of fields on the configuration singleton is getting a bit out of hand, so for that discussion we could also consider splitting the model, like in technical system configuration, content options, layout stuff and texts.
This could also make progress to getting rid of that crazy context-processor that just copies a pile of data from the singleton into a dictionary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. We should come up with a good division though, lest we end up proliferating configuration types and creating something worse.
8edfb29
to
81ba2e0
Compare
a9829f8
to
bd7d872
Compare
Jiro wants to check the weird thing with italic stuff |
@@ -60,6 +60,9 @@ def delete_model(self, request, obj): | |||
else: | |||
super().delete_model(request, obj) | |||
|
|||
class Media: | |||
css = {"all": ("css/admin/admin_overrides.css",)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why (as it was working before) but when I upload any kind of TTF file, it does correctly end up in the Media directory, but I do not see anything happening on the front-end (perhaps we need a fallback font somewhere?).
Here you can download clearly different TTF fonts for testing: https://www.fontsquirrel.com/fonts/acme
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The font faces definitions were missing the custom fonts after rebasing. Also, I had to specify the font-family for our utrecht-heading
. Should work now.
bd7d872
to
13ad94d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately the rendering does not work properly for Bold/Italic font styles if they aren't uploaded as well - the 'faux' rendering makes font-styling barely visible in Chrome and even strips the Bold styling from Card titles, so I'd say: only for the Body font you'll need to add upload options for ""regular"/"italic"/"bold"/"bold-italic". But keep the font-weight: bold;
and font-style: italic;
definitions for each individual font as needed.
In general: "every browser is doing its own thing in font rendering. ... Every browser seems to take its own approach to rendering font weight and anti-aliasing." Using a native bold-face and italic-face is the best way to counter browser differences.
I don't think it is necessary to add "italic" or "bold" uploads for Headings since this should be rendered as a different font anyway, so For Headings you can remove my original bold/italic definitions and probably only use:
@font-face { font-family: 'Heading'; src: url('/media/custom_fonts/heading_font.ttf') format('truetype'), url('/static/fonts/ubuntu/Ubuntu-M.ttf') format('truetype'); }
13ad94d
to
cc3218d
Compare
FAILED (failures=3) |
cc3218d
to
c5bae27
Compare
c5bae27
to
735c79d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these tests seem to fail since #963 was merged, possibly related to the App.scss font-face changes
these tests seem to fail since #963 was merged, possibly related to the App.scss font-face changes
these tests seem to fail since #963 was merged, possibly related to the App.scss font-face changes
Taiga #2030