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

feat: add custom font icons support #28

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

P0labrD
Copy link
Collaborator

@P0labrD P0labrD commented Jul 25, 2024

Why

Inspired by Golden toolkit font_loader.dart use of FontManifest.json that references all fonts by packages in build/unit_test/assets folder

Use loadAppFonts instead of loadFonts or loadFontsFromPackage

Todo

  • add usage examples
  • update README usage
  • update README goldens screenshots on README

/// * Create a flutter_test_config.dart file. See:
/// https://api.flutter.dev/flutter/flutter_test/flutter_test-library.html
/// * add `await loadAppFonts();` in the `testExecutable` function.
Future<void> loadAppFonts() async {
Copy link
Member

Choose a reason for hiding this comment

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

Le mot "app" fait un peu four tout.

  • Soit on trouve un meilleure naming, genre loadFontFromManifest ou autre.
  • Soit on fait un breaking change. Est-ce que loadFont est encore pertinent ? Sinon est-ce qu'on a besoin de maintenir cette fonction ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

À priori, je ne vois pas d'utilisation de loadFonts que loadAppFonts ne couvre pas ; le breaking change me semble mieux.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@MaximeRougieux un avis sur la question?

Copy link
Contributor

Choose a reason for hiding this comment

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

Go breaking change, le package est en beta donc c'est ok

);
final waitList = <Future<void>>[];
for (final Map<String, dynamic> font in fontManifest) {
final fontLoader = FontLoader(font['family']);

Choose a reason for hiding this comment

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

I see you didn't add the function for derivedFontFamily here. Why did you miss it?
Screenshot 2024-07-27 at 14 26 42

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, I don't understand what you're refering to. Is derivedFontFamily a function from a package? Or is it a proposition to extract some logic?

If it is the first, can you link a doc link to the function, because I couldn't find it anywhere I looked

Choose a reason for hiding this comment

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

It is here: font_loader.dart
Method loadAppFonts is moved from flutter_glove_box project.

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