Skip to content

Conversation

@guidezpl
Copy link
Member

In all but two places, we can use the google_fonts package.

Merging is blocked on material-foundation/flutter-packages#58 (for web support)


flutter:
assets:
- fonts/google_fonts/
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

The google_fonts package will first check assets for font files before fetching them and caching them. See https://github.com/material-foundation/google-fonts-flutter/blob/1255da8321fb84be2ffe56fcc5f792eb1dadf83d/README.md#bundling-font-files-in-your-applications-assets for more details

Copy link
Member Author

Choose a reason for hiding this comment

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

We could chose not to bundle them, but then you'd get a flicker as fonts load the first time, and the fonts wouldn't load if you opened the app offline for the first time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, the readme mentions that the directory should be - google_fonts/, but I guess ours works with the extra level in there? Or should we modify that too

Copy link
Member Author

Choose a reason for hiding this comment

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

It can use any directory. I just figured it made more sense to put them under fonts/


flutter:
assets:
- fonts/google_fonts/
Copy link
Contributor

Choose a reason for hiding this comment

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

Also I see all the fonts are still included in the directory, is that how it's supposed to work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, see above

Copy link
Contributor

@rami-a rami-a left a comment

Choose a reason for hiding this comment

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

LGTM but I think we should get @clocksmith or @johnsonmh to review this one also.

Copy link

@johnsonmh johnsonmh left a comment

Choose a reason for hiding this comment

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

██╗      ██████╗ ████████╗███╗   ███╗
██║     ██╔════╝ ╚══██╔══╝████╗ ████║
██║     ██║  ███╗   ██║   ██╔████╔██║
██║     ██║   ██║   ██║   ██║╚██╔╝██║
███████╗╚██████╔╝   ██║   ██║ ╚═╝ ██║
╚══════╝ ╚═════╝    ╚═╝   ╚═╝     ╚═╝

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