Repository navigation
Conversation
github-actions
Bot
added
the
tool
|
Greetings from stale PR triage! 👋 |
260e8f0 to
b2b01f8
Compare
|
@bkonyi @dcharkes @goderbauer wdyt? Just a proof of concept. The next step would be to give |
a53ba50 to
771469c
Compare
373ba17 to
3af6ccc
Compare
mosuem
changed the title
dcharkes
left a comment
dcharkes
left a comment
There was a problem hiding this comment.
Interesting!
I haven't looked at all the wiring, and if the caching works correctly with the targets etc.
This PR would benefit from an example (or better, an integration test as example).
I left some more remarks on the API design and naming.
| ffi: 2.1.4 | ||
| file: 7.0.1 | ||
| flutter_template_images: 5.0.0 | ||
| font_asset: 0.0.4 |
There was a problem hiding this comment.
This should have been font_assets to be consistent with code_assets and data_assets.
There was a problem hiding this comment.
Yeah I noticed that afterwards as well.. 🙃
|
|
||
| class FontHookAsset extends HookAsset { | ||
| FontHookAsset({ | ||
| required super.file, |
There was a problem hiding this comment.
This doesn't seem to mirror the way the yaml is set up. The yaml has more than one file per font.
https://docs.flutter.dev/cookbook/design/fonts#define-fonts-in-the-pubspec-yaml-file
Would it make more sense to make a FontFamilyAsset with a list of Fonts? In the current setup you'll end up repeating the family name. Especially if you end up writing a package:font_assets FontAsset object literal, it would make sense if it 1-to-1 translates to the yaml structure.
Also, I see a weight, but not an optional style?
There was a problem hiding this comment.
Yes, I just went with a simple setup first to test it.
There is https://github.com/mosuem/font_asset_test, but I added that as an example here instead. |
github-actions
Bot
added
a: desktop
|
Sorry, not sure how this slipped past me! I'm curious, do we plan on having developers specify their fonts in build hooks rather than the pubspec moving forward? Or do we plan on maintaining the current workflow of specifying fonts within the pubspec but generating and running a fonts build hook at runtime? |
|
I believe a custom The only advantage that the Flutter SDK knowing about I think the way to make that work is to send a If we can make such pattern work, we don't need to add SDK support for custom asset types, they can be built on |
|
How would you merge fonts declared in the pubspec with fonts declared through the flutter_framework package? |
Eventually, we'd phase those out? Theoretically. 😄 If we keep supporting them, we don't have to use font assets for the implementation of fonts from hooks. One small issue I can think of is that we wouldn't notice a conflict between built-in fonts and fonts loaded at runtime via data assets until runtime. Probably not a super often occurrence, so that might be acceptable. If we want to phase support out, we could provide a hook that simply reads from the pubspec. Though users would still have to migrate. Actually the migration path should then maybe be that conflicts are not conflicts but that the new way overrides the oldway. |
|
Greetings from triage! Is this a change we would like to move forward with? :) |
|
I would like to move forward with this, I still believe it is the right path. I do not have the time right now unfortunately... |
Totally understand! This has made it back to stale triage. Is it something we need to leave open in the meantime? |
That would be great! |
|
Welcome back to stale triage! Should we close this for now? It has landed in stale triage many times now over the last year. If it is not actively being working on, perhaps we should close it. It can always be re-opened later. |
|
Yeah it has unfortunately accumulated many merge conflicts by now, which is why I try to not let PRs get stale :/ Closing it, maybe someone else wants to tackle this sometime. |
4 participants
Allows to add fonts via build hooks with https://pub.dev/packages/font_asset, see https://github.com/mosuem/font_asset_test as an example.
This now also calls the icon treeshaker via a link hook.
TODO:
app.dilland other binaries.@RecordUseinstead of custom logic.Long term: Unbundle icontreeshaker from flutter, remove fonts via pubspec adding.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.