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

TW-824: install google fonts to fetch font from http #832

Merged
merged 2 commits into from
Oct 23, 2023

Conversation

imGok
Copy link
Contributor

@imGok imGok commented Oct 17, 2023

@imGok imGok temporarily deployed to PR-832 October 17, 2023 13:43 — with GitHub Actions Inactive
@github-actions
Copy link

This PR has been deployed to https://linagora.github.io/twake-on-matrix/832

@imGok imGok temporarily deployed to PR-832 October 17, 2023 14:14 — with GitHub Actions Inactive
@imGok imGok temporarily deployed to PR-832 October 17, 2023 14:15 — with GitHub Actions Inactive
@imGok imGok force-pushed the TW-824-install-google-fonts-http branch from 1b6db39 to db06315 Compare October 17, 2023 14:43
@imGok imGok temporarily deployed to PR-832 October 17, 2023 14:43 — with GitHub Actions Inactive
@imGok imGok changed the title [WIP] TW-824: install google fonts to fetch font from http TW-824: install google fonts to fetch font from http Oct 17, 2023
web/style.css Outdated Show resolved Hide resolved
@drminh2807
Copy link
Contributor

Screenshot 2023-10-18 at 08 34 29 Screenshot 2023-10-18 at 08 34 39

Better
but it is from linagora.github.io, it can slower on beta.twake.app
I wonder why we still need Inter font in assets
I read it https://pub.dev/packages/google_fonts#bundling-fonts-when-releasing
but I think it good for mobile or desktop, but on web it should download fonts after first loaded
does flutter support it ?

Copy link
Member

Choose a reason for hiding this comment

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

how about to remove all font files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with assets, the package will first look into assets folder then if not present it will fetch.
without assets it will http fetch on first loaded then cache it (no more http font requests after first loading)

without assets
image
with assets
image

@imGok imGok temporarily deployed to PR-832 October 18, 2023 14:00 — with GitHub Actions Inactive
@imGok
Copy link
Contributor Author

imGok commented Oct 18, 2023

@drminh2807 I think it's possible if we use only googlefonts package for web only and do a condition in themes.dart. But is it a good practice?

@hoangdat
Copy link
Member

@drminh2807 I think it's possible if we use only googlefonts package for web only and do a condition in themes.dart. But is it a good practice?

let's try :)))

@imGok imGok changed the base branch from main to feat/flutter-3-13 October 19, 2023 13:19
@imGok imGok force-pushed the TW-824-install-google-fonts-http branch from edbd141 to 85ac777 Compare October 19, 2023 14:27
@imGok imGok temporarily deployed to PR-832 October 19, 2023 14:27 — with GitHub Actions Inactive
@imGok
Copy link
Contributor Author

imGok commented Oct 19, 2023

@hoangdat added ADR

@drminh2807
Copy link
Contributor

@drminh2807 I think it's possible if we use only googlefonts package for web only and do a condition in themes.dart. But is it a good practice?

you still keep font in pubspec, so web still download font files from assets

Screenshot 2023-10-20 at 08 14 27

platform specific asset is not supported yet in flutter
flutter/flutter#8230
So you should reverse back to yesterday
we will improve add assets/google-fonts later

sherlockvn
sherlockvn previously approved these changes Oct 20, 2023
@imGok imGok force-pushed the TW-824-install-google-fonts-http branch from 85ac777 to 32211f6 Compare October 20, 2023 08:58
@imGok imGok temporarily deployed to PR-832 October 20, 2023 08:58 — with GitHub Actions Inactive
@imGok
Copy link
Contributor Author

imGok commented Oct 20, 2023

@drminh2807 reversed to normal implemention of google fonts, please check again:)

drminh2807
drminh2807 previously approved these changes Oct 23, 2023
Base automatically changed from feat/flutter-3-13 to main October 23, 2023 04:00
@hoangdat hoangdat dismissed stale reviews from drminh2807 and sherlockvn October 23, 2023 04:00

The base branch was changed.

@hoangdat hoangdat merged commit 332a090 into main Oct 23, 2023
3 checks passed
@hoangdat hoangdat deleted the TW-824-install-google-fonts-http branch October 23, 2023 08:11
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.

4 participants