Feature: Bring your own font #3020
Conversation
There was a problem hiding this comment.
Overall a great start. I didn't pull it down and test it, but from the code, I got the jist of it. I think this will be a great enhancement, but I do strongly think moving to a more UI centered loading system would be really smart. This will reduce friction by having to have admin's add their own fonts via their filesystem and instead rely completely on the UI.
I would also argue that we could expand this functionality to non-admins (we would need a role for this). And if we really wanted to go to the next step, we could intake not only a file, but a Google Fonts url and download directly into the fonts directory.
Let me know your thoughts as that expands the scope (but would be really fun to code).
|
|
||
| [ResponseCache(CacheProfileName = "10Minute")] | ||
| [AllowAnonymous] | ||
| [HttpGet("GetFonts")] |
There was a problem hiding this comment.
This is not how API names are. Use either list or all
| } | ||
|
|
||
| [AllowAnonymous] | ||
| [HttpGet("download-font")] |
There was a problem hiding this comment.
I would leave this as HttpGet only. So api is /api/font.
This need authentication via API Key. We cannot have an exposed api to allow downloading fonts off the server.
| var font = await _unitOfWork.EpubFontRepository.GetFont(fontId); | ||
| if (font == null) return NotFound(); | ||
| var contentType = GetContentType(font.FileName); | ||
| return File(await _fontService.GetContent(fontId), contentType, font.FileName); |
There was a problem hiding this comment.
Use PhysicalFile like other areas an use enable Range processing.
| try | ||
| { | ||
| var font = await _unitOfWork.EpubFontRepository.GetFont(fontId); | ||
| if (font == null) return NotFound(); |
There was a problem hiding this comment.
I like to have a newline above if statements and return statements to help keep code easy to consume.
| { | ||
| try | ||
| { | ||
| var font = await _unitOfWork.EpubFontRepository.GetFont(fontId); |
There was a problem hiding this comment.
Although I don't follow always, having Async when the method returns a task is very helpful.
| fonts.forEach(font => { | ||
| const fontFace = new FontFace(font.name, `url(${this.bookService.baseUrl}Font/download-font?fontId=${font.id})`); | ||
| fontFace.load().then(loadedFace => { | ||
| (document as any).fonts.add(loadedFace); |
There was a problem hiding this comment.
document should come from the injected Document instance, to allow SSR if I ever want to go that route.
| this.bookService.getEpubFonts().subscribe(fonts => { | ||
| fonts.forEach(font => { | ||
| const fontFace = new FontFace(font.name, `url(${this.bookService.baseUrl}Font/download-font?fontId=${font.id})`); | ||
| fontFace.load().then(loadedFace => { |
There was a problem hiding this comment.
One thing I'm worried about is timing. There is no way to know if all the fonts are loaded or not. I also don't see setting the loaded fonts somewhere for the user to select from.
There was a problem hiding this comment.
Second pass, I think instead what we should do is move this load fonts to the book service. To make this most efficient, we can cache the loaded fonts and return the fonts from that.
bookService.getEpubFonts() {
getFonts().pipe(tap(f => {
const fontFace = new FontFace(font.name, `url(${this.bookService.baseUrl}Font/download-font?fontId=${font.id})`);
fontFace.load().then(loadedFace => { // cache then put on the document }
// ...
}We just need to bust this when we get a fontScan complete (or if we go the theme manager route, book theme modified event).
Thoughts?
| User = 2, | ||
| } | ||
|
|
||
| export interface EpubFont { |
There was a problem hiding this comment.
These should be in their own ts file.
| return [{title: 'default', family: 'default'}, {title: 'EBGaramond', family: 'EBGaramond'}, {title: 'Fira Sans', family: 'Fira_Sans'}, | ||
| {title: 'Lato', family: 'Lato'}, {title: 'Libre Baskerville', family: 'Libre_Baskerville'}, {title: 'Merriweather', family: 'Merriweather'}, | ||
| {title: 'Nanum Gothic', family: 'Nanum_Gothic'}, {title: 'RocknRoll One', family: 'RocknRoll_One'}, {title: 'Open Dyslexic', family: 'OpenDyslexic2'}]; | ||
| getEpubFonts(): Observable<EpubFont[]> { |
There was a problem hiding this comment.
We don't need to specify the return on the method signature since it's now obv.
| this.bookService.getEpubFonts().subscribe(res => { | ||
| this.fontFamilies = res.map(f => f.name); | ||
| this.cdRef.markForCheck(); | ||
| }) |
Added
THIS IS A DRAFT
I'm not done with this yet, quite a bit to do. But feel like it is time to ask for feedback on a few things. Opening a draft pull request, just so it's here as well. Will cross post in the discord after.
Loading the fonts.
They're currently being loaded in the book-reader, this should ensure they're only being loaded when needed. I've also not noticed any higher loading times, but I've only had max 4 fonts loaded. We may want to try with a bunch more, at the very least the default ones, to see if it has any noticeable impact.
Design choices
Need some feedback / greenlight for design choices as to call what what, and place what where.
EpubFonts${pwd}/config/fontsIf these are fine, how do we want to differentiate between the default fonts, and user fonts? Can we store them in the same folder, should we not?
If these are not fine, suggestions as to changes to do.
Link with AppUserPreferences
Currently, the font preference for a user is saved, as the fonts name. It's still doing this currently, and should work fine. However, we could change this to the ID of the font. Having it be the fonts name, does make it easier to load and reset them. As simple as changing it to default.
Default font
The dropdown menu in the UI, currently, isn't displaying the default font, as it's not an EpubFont. Where should I best inject this option? In the UI, or the controller; instinctively I would do it in the controller, such that the UI doesn't have too much extra logic it has no business of owning. But wanted to ask for opinions anyway, maybe there is a better place?
Notes
TODO's
Apart from "smaller" TODO's in the code. I think these are the big ones still to do
I may have missed some