search

skb1129 / react-native-change-icon

Change your application icon programmatically for React Native apps
MIT License
500 stars 91 forks source link

Types, Readme + Spring Cleaning #86

Closed haydncomley closed 1 year ago

haydncomley commented 1 year ago

Hello! First off, love the package and I use it within a couple of my projects, however there are some issues throughout. Rather than keep the additions I made to myself I've tried to put them all in a nice PR for you and others to potentially consume. I've gone through and fixed the issues others have mentioned with Android crashing. At the same time I've added types and created a helper function for resetting the icon to the default.

While I've gone through the code, I thought it was worth also updating the Readme.md to be a but more helpful for those that aren't sure where to begin on the graphics side of this. I've included examples, and screenshots to explain visually what happens at each step. I have also remove the example project itself as I thought this wasn't needed and the instructions alone should be good for a user to follow.

Hope this PR looks like a positive change for the repo as it hasn't been touched in a while and is the main one that comes up when people google react native change icon (obviously)!

skb1129 commented 1 year ago

Hey @haydncomley, First of all, thanks for this PR, this makes a lot of great changes to the project. Just one thing that I'd like to change here is TypeScript. We should just provide type definitions for the apps that use TypeScript, but the package itself should not depend on TypeScript.

haydncomley commented 1 year ago

Totally agree, this should be the case as the dist folder is included and includes the built output as .js with mapping files included - then within the package.json it notes where to pull X from for the different dev environments.

Screenshot 2023-08-17 at 16 40 48 Screenshot 2023-08-17 at 16 41 26

If I've got this confused or wrong though it's your package so feel free to make any changes or additions!

skb1129 commented 1 year ago

@haydncomley That part I understood, no issues there, the problem is the dependencies, you've added typescript in "dependencies" which would mean they would end up in all the bundles, regardless if the project is using typescript or not, it should be in "devDependencies".

haydncomley commented 1 year ago

Oh yes you're right! Great catch - when I get home I can push some updates for you 🫡

haydncomley commented 1 year ago

I've just updated the deps for you. I also changed back the setter to setIcon rather than changeIcon (not sure why I changed this in the first place)