-
Notifications
You must be signed in to change notification settings - Fork 248
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
Flutter Favorites Todos #133
Comments
Null safety changes are tracked in the null-safety branch |
I'm not sure what to do about #31. We could decrease the size of each icon by multiplying it with .75 to get comparably sized icons. This will however be a little strange/a breaking change for existing users. Would love to hear your opinion on this @brianegan |
Yah, that's definitely a tough one! From my perspective, I don't think we can truly solve this problem, which is why I initially closed the issue as a wontfix. Why do I think that? Icon packs are designed with specific geometries and shapes in mind so that the icons within a specific Icon Pack are consistent with one another. Material Icons and FontAwesome Icons do not share a common geometry: All Material Icons are all set within a square of a particular size, FontAwesome are not -- they can be rectangles! Now, you could say "Let's apply a 75% size ratio to make them look similar." However, there's still no way for the FontAwesome icons to follow that fundamental square geometry, so some will look correct at 75%, but some Icons that are a bit wide or tall may still look a bit big or even too small. Therefore, I still consider it a wontfix, but if we want to do something there, then maybe we could add an option for folks to override? Such as "iconThemeRatio" that is default to 1 so we don't break backward compatibility, but for folks who want to they could adjust that number to 0.75? |
The different geometries are defenitely a wontfix, it's just part of how the icons are designed. I really like your proposal of an override attribute. However as this is probably a niche requirement that can be solved with a wrapper widget as well and in the spirit of keeping the api clean, I agree it's better we leave it as a wontfix |
Agreed. Wrapper Widgets FTW here! Really amazing work with all of this. Thanks again for all of your efforts :) |
Thanks! The only thing I couldn't figure out was how to fix the alignment issue, I'm definitely going to need your help there |
Sure thing! I'm wrapping up work this week from my side. Therefore, I'll have time to check it out early next week :) |
Icon
class toFaIcon
,FaDuotoneIcon
(Support null safety. #134)The text was updated successfully, but these errors were encountered: