-
-
Notifications
You must be signed in to change notification settings - Fork 150
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
Include image dimensions in generated AssetGenImage class #431
Conversation
If you already have a solution for images, why SVGs are supported only? |
Sorry, which solution for images? The Or are you asking why I haven't done this for images? I only needed it for svgs, but I'm happy to also add it to images (and Rive/Lottie), but I wanted to limit my work until you already did an initial review of the concept. If this idea sounds good I'll add other support tomorrow |
In general this looks great. Looking forward to further integrations. |
dd9de6c
to
4abf8a0
Compare
Just rebased onto the latest main branch. Now adding changes for parsing other asset types. It would simplify the code if you review #432 first, which refactors the default asset types out into its own |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #431 +/- ##
==========================================
+ Coverage 98.29% 98.34% +0.05%
==========================================
Files 21 21
Lines 702 724 +22
==========================================
+ Hits 690 712 +22
Misses 12 12 ☔ View full report in Codecov by Sentry. |
Ok |
@AlexV525 any chance of moving this forward? Or what can I do to progress this? Thanks |
Sorry I need some time to coordinate my open-source working time. I'll get back to review once I'm available. |
Ok thanks. I'll check in periodically, and if there is anything I can do to help, please ask. |
@@ -26,6 +26,8 @@ dependencies: | |||
dart_style: ^2.2.4 | |||
args: ^2.0.0 | |||
pub_semver: ^2.0.0 | |||
vector_graphics_compiler: ^1.1.9 | |||
image_size_getter: ^2.1.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, our package, great. :)
Thanks. I'm about to be ooo for a week, but I'll fix up what's needed when I return. |
Ok rebased onto the latest main, and addressed the reviewed comments. Please take another look @AlexV525 thanks! |
@bramp The implementation looks good. Consider adding a doc entry in the README? |
…tead of a string path. This enables the Integration to be smarter with what they do with the asset. This also included refactoring how and where posixPath is used. This potentially leads to less error prone usage.
…e additional asset information in the generated file.
… width/height in the generated assets.gen.dart.
Ok updated the README, and resynced to fix conflict with main. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with 1 nit picking
Co-authored-by: Alex Li <[email protected]>
What does this change?
This adds a new config
parse_metadata
which at build time determines the dimensions of image assets and includes them in the generatedAssetGenImage
class.Before this change you'd need to do this: (taken from StackOverflow):
After this change you can now easily do
So far this only works for SVGs, but it'll be easy to add the other types, and include other metadata (not just dimension if deemed useful).
Fixes #402🎯
Type of change
Please delete options that are not relevant.
Checklist:
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
melos run unit:test
) <-- I think this is justmelos test
melos run format
to automatically apply formatting)