-
Notifications
You must be signed in to change notification settings - Fork 32
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
Allow disabling cdn from datahub and datafeeder #857
Conversation
Affected libs: ``,
|
📷 Screenshots are here! |
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.
Why not make all these fonts local once and for all? this shouldn't change anything for users using the default fonts as well as for the ones using custom fonts.
edit: what I mean is that we could just copy the CSS stylesheet in the assets along with the fonts, and then reference this stylesheet instead of the remote one.
6f2fd94
to
9949493
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.
With the current PR the Datahub and Editor apps still rely on google fonts, because of this:
geonetwork-ui/apps/datahub/src/app/app.module.ts
Lines 216 to 217 in 6083baa
getThemeConfig().FONTS_STYLESHEET_URL || | |
'https://fonts.googleapis.com/css2?family=Readex+Pro&family=Rubik&display=swap' |
The stylesheet used for fonts should be configurable and not hardcoded in the HTML (this is not the case for icons font, which is always loaded no matter what). When I remove the google fonts stylesheet, the default-fonts.css stylesheet doesn't work because the paths in the CSS are not right.
I'm also wondering about the format; I feel like using "woff2" fonts would make more sense as it is a format optimized for the web. When looking at https://fonts.googleapis.com/css2?family=Readex+Pro&family=Rubik&display=swap (the default font stylesheet), there is also much more to it than just the current content of default-fonts.css. We should maybe just copy all this into the assets of the project, just to make sure we don't have invalid characters somewhere down the road?
Thanks!
When we hit https://fonts.googleapis.com/css2?family=Readex+Pro&family=Rubik&display=swap there are some font face for arabic, cyrillic, hebrew and vietnamese chars. Should we implement them also ? As they are not implemented in those apps. I can implement the woff by retrieving from the url provided in this css file but, when we download it from the google font official link, I got only the ttf. Is it ok for you if I put only an empty string ?
|
Honestly I think you can just copy paste the whole CSS and not worry to much about it; we don't have such languages for now but there's no reason for it to stay that way (e.g. core-geonetwork now supports georgian, ukrainian etc.).
I would go for woff2 to replicate the behavior that we had before, just to make sure we don't break anything. thanks :)
I'm not sure I see the logic; users can decide to use a different stylesheet with different fonts, why would we also include the default one then? I would keep the same behaviour as before, e.g. pointing on a default stylesheet. |
ad48200
to
b33ed41
Compare
Co-authored-by: Olivia Guyot <olivia.guyot@camptocamp.com>
926d3c6
to
55d7b20
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.
I have added a commit with a few fixes and some doc, and rebased on main (to hopefully fix the e2e test failing)
Merge when you see fit :) PS: this should be squashed
55d7b20
to
78030f9
Compare
Description
This PR introduces removes CDN resources of Datahub, Datafeeder and Metadata-editor.
Quality Assurance Checklist
This work is sponsored by MEL.