-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
New Major Version feedback #1638
Comments
Hi @vonovak. Thanks for the feedback, apologies for the delay, I've been travelling.
That's a terminology misstep on my part. I do mean the native binary size. I'll update that everywhere. NOTE: Those are the old installation instructions you linked to. See the new branch: https://github.com/oblador/react-native-vector-icons/tree/monorepo?#installation. In the new version, there is a script that copies only the fonts for installed packages into the bundle. iOSThe key goal was to simplify installation and make it zero-touch. The majority of the project's user issues are caused by installation problems. Based on some of the documentation you've shared, I think I might be overreaching here. The script on iOS is mainly to avoid needing the podspec in every module, the fonts are auto generated though so this is not a biggy and we could include one. Do you think it would be possible to either make use of some expo-font code or use it directly in some way as part of this library to not require users to edit Thanks for the gist, I'll definitely look at improving my hack :) Android
It doesn't copy them into the directory mentioned there, as that would mean they need to be git ignored or committed. The scripts place them in Thanks for the feedback! |
Hi @johnf! You mentioned that most issues people have on iOS are around installing - I believe the installation instructions can be much simpler. I made a PR for that: #1636
If the script that does the copying is solid, then I agree there's no need for podspec (again, just for completeness: how cocoapods do it).
It'd be nice to not have to change why rely on changed in `info.plist`With the current There are cases when users might want to load a font family dynamically (without changes to Hope this makes sense, and thank you for explaining everything. |
@vonovak I've just rolled a new release that moves back to a more static treatment of fonts on iOS |
Thank you for making that change; that sounds like a good way to handle it! |
@vonovak Are there any more pull requests coming? I's like to cut a new alpha release soon |
Hi @johnf yes I'll open 1 PR tomorrow. |
@vonovak Thanks for all the help with the dynamic font loading and other improvements. |
hey @johnf I was wondering if you have a timeline for releasing stuff from the Are there todo items / tasks that need resolving and would move us closer to that goal? Also regarding 82b4fda I just wanted to point out that the naming of Thank you! 🙂 |
@vonovak I'm hoping to release a beta this weekend and then a full version in a couple of weeks. Regarding the naming, you are right. I'm going to switch it back. I think it's the leftover rubyisms in my brain that preferred filename, but FileName seems more correct in JS/TS land |
@johnf great to hear that! Rather, the issue with it is that Detox supports specific React Native versions and doesn't officially work with the new architecture so it might block you from testing the most recent releases. I'm not too familiar with react native owl so I cannot comment on it but Appium (as much as I don't like it because to me it seems it's trying to do a lot of things but only few work really properly) or Maestro (which I don't have experience with but heard positive comments on) would seems better choices for this purpose compared to Detox. |
hi, quick comment these instructions didn't work for me (with the new per-package approach) it can't find i manually added the fonts i installed to info.plist |
Hi!
Let me start by saying thanks for maintaining the library! :) I'm reaching out with regard to #1612 to provide some feedback for the new major version rewrite. I figured it'd be easier to open a new issue to keep discussion more focused.
Firstly, a clarifying question about "Package per font - Only install the fonts you need for smaller bundle sizes" which is listed as one of the benefits of the rewrite.
Does this concern the JS bundle size or the native binary size (because the "default" installation instructions result into copying all icon files into the binary)?
In terms of JS bundle size, I believe there shouldn't be a difference, because metro will bundle only the JS file that are imported in the JS code, so if I import
import Icon from 'react-native-vector-icons/Ionicons'
, then only the JS code backingIonicons
will be bundled.Secondly, I'd like to provide some feedback on the implementation. Please correct me if I'm wrong somewhere.
To render custom fonts, on both platforms, the font files need to be copied into the native app. This is all that needs to happen on Android, afaik. Then on iOS, the fonts also need to be added to
Info.plist
.How font setup happens now
iOS
The copying of font files is currently done by this line in the podspec. The iOS installation instructions currently say that a manual step is needed, but I haven't found it to be necessary. In fact, when I follow the "iOS Setup" I get an error
Multiple commands produce '.../iconsdemo.app/FontAwesome6_Regular.ttf'
. I tried to improve the installation instructions in #1636.Then,
UIAppFonts
need to be added toInfo.plist
.Android
The
fonts.gradle
copies the font files. That's it.How font setup happens in the new monorepo branch
iOS
The font file copying part is no longer taken care of by Cocoapods but by a custom bash script here.
Note that because the font files are copied into a
react-native-vector-icons
subfolder in the application bundle, the icon fonts won't render correctly even if they are added in theUIAppFonts
entry inInfo.plist
. This I find unexpected and at odds with how apple covers adding custom fonts.Icon rendering works in the end thanks to this
loadFont()
call. However, if standard setup was followed this call wouldn't be necessary. The implementation is necessary for dynamic font loading but for static fonts, the call doesn't need to happen. Let me also note that the promise returned by the call is not awaited so race conditions could happen when icon is rendered before the font is loaded.The aforementioned bash script calls the
getFonts.js
node script which readspackage.json
and looks for allreact-native-vector-icons
subpackages and collects the font files from them. It appears it should work okay with monorepos 👍 .Android
The
getFonts.js
script is called bybuild.gradle
Question for Android: With the new implementation, are the font files copied to the location documented here?
Summary
info.plist
. (For Expo users, this manual setup is not needed when using the Font config plugin)The text was updated successfully, but these errors were encountered: