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.

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

Merged
merged 7 commits into from
Apr 23, 2024
Merged

Conversation

f-necas
Copy link
Collaborator

@f-necas f-necas commented Apr 16, 2024

Description

This PR introduces removes CDN resources of Datahub, Datafeeder and Metadata-editor.

Quality Assurance Checklist

  • Commit history is devoid of any merge commits and readable to facilitate reviews
  • The documentation website 📚 has received the love it deserves

This work is sponsored by MEL.

Copy link
Contributor

github-actions bot commented Apr 16, 2024

Affected libs: ``,
Affected apps: datafeeder, `datahub`, `metadata-editor`,

  • 🚀 Build and deploy storybook and demo on GitHub Pages
  • 📦 Build and push affected docker images

@coveralls
Copy link

coveralls commented Apr 16, 2024

Coverage Status

coverage: 83.162% (-2.0%) from 85.163%
when pulling 78030f9 on allow-discard-cdn
into c22cb78 on main.

Copy link
Contributor

github-actions bot commented Apr 16, 2024

📷 Screenshots are here!

Copy link
Collaborator

@jahow jahow left a 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.

apps/datahub/src/index.html Outdated Show resolved Hide resolved
@f-necas f-necas requested a review from jahow April 16, 2024 12:46
Copy link
Collaborator

@jahow jahow left a 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:

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!

@f-necas
Copy link
Collaborator Author

f-necas commented Apr 19, 2024

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.
Also I think this ttf implement all chars instead of google's woff2 which seems to be small parts of the font. Indeed they should be more optimized and a lot smaller.

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 ?

      getThemeConfig().FONTS_STYLESHEET_URL || ''

@jahow
Copy link
Collaborator

jahow commented Apr 20, 2024

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. Also I think this ttf implement all chars instead of google's woff2 which seems to be small parts of the font. Indeed they should be more optimized and a lot smaller.

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 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.

I would go for woff2 to replicate the behavior that we had before, just to make sure we don't break anything. thanks :)

Is it ok for you if I put only an empty string ?

      getThemeConfig().FONTS_STYLESHEET_URL || ''

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.

Copy link
Collaborator

@jahow jahow left a 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

@f-necas f-necas merged commit a288b23 into main Apr 23, 2024
9 checks passed
@f-necas f-necas deleted the allow-discard-cdn branch April 23, 2024 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants