Skip to content

Add Development information to readme#378

Open
EricGDevel wants to merge 1 commit into
salemlf:mainfrom
EricGDevel:update_development_readme
Open

Add Development information to readme#378
EricGDevel wants to merge 1 commit into
salemlf:mainfrom
EricGDevel:update_development_readme

Conversation

@EricGDevel

Copy link
Copy Markdown

Hello!
Hakubun has inspired me to finally start contributing to open-source projects, and try out a new field of programming.
In this PR I add a little bit of information to the development section in the README, that would have saved me time starting out, and which will hopefully make it easier for new contributors to join.

Also, I saw that you have a stale branch that adds a bit more information about the app's architecture to the README, and I think it would be great if you also merged that branch.

@EricGDevel EricGDevel requested a review from salemlf as a code owner June 23, 2024 16:36
Comment thread README.md Outdated

#### iOS and Android Simulators (with Hot Reload)

Make sure to install Corvoda's required dependencies for [iOS](https://cordova.apache.org/docs/en/11.x/guide/platforms/ios/index.html)/[Android](https://cordova.apache.org/docs/en/11.x/guide/platforms/android/) before running the following commands.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, could totally be incorrect, but I don't believe you should have to install these manually. Running npm install should be sufficient to install all the necessary dependencies, did you run into issues using that method?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

npm install installed all of the dependencies I needed except for the Android SDK, which I installed with Android Studio.
I think having these links could be convenient in the event that someone is struggling with the dependencies, so they could read about what they are missing, and I will mention Android Studio explicitly.
Do you think I should change the phrasing as well?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh yeah okay, that makes sense! I think maybe changing the phrasing to something like "if you're having issues running the iOS/Android simulators, you may have to install these dependencies"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also encountered issues trying to run the iOS and Android servers locally, so I think these extra bits of documentation will be helpful! (That said, I haven't fully tried it yet)

P.s. in case the original wording is kept in some form, there's a typo: Corvoda -> Cordova

Comment thread README.md Outdated

# If any errors occur, set the following environment variables and try again, or run the app using Android Studio
export JAVA_HOME=/path/to/android-studio/jbr
export ANDROID_SDK_ROOT=/path/to/Android/Sdk

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a super helpful addition, thanks! 😊 Would you mind changing these to this syntax? I just call it "insert here" syntax haha, probably has a technical term.
Syntax:
export JAVA_HOME=<PATH_TO_JAVA_HOME>
And the same idea for the syntax of ANDROID_SDK_ROOT variable.

It'd also be nice to have some info on how to get the location for Java home easily, maybe something like:
Windows: echo %JAVA_HOME%
Mac/Linux: echo $JAVA_HOME

And for ANDROID_SDK_ROOT, that's typically in these locations:
Mac: /Users/<USERNAME>/Library/Android/sdk
Windows: C:\Program Files\Android\sdk OR C:/Users/<USERNAME>/AppData/Local/Android/Sdk

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't currently have a ~/Library/Android directory - would installing the Cordova dependencies fix that?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yndajas

I don't currently have a ~/Library/Android directory - would installing the Cordova dependencies fix that?

The ~/Library/Android path is referencing where my Android SDK is installed, it may be different for your device. I don't believe the Cordova dependencies should affect that. Instructions on how to install Android SDK can be found here.

Comment thread README.md Outdated
Comment on lines +190 to +193

# If any errors occur, set the following environment variables and try again, or run the app using Android Studio
export JAVA_HOME=/path/to/android-studio/jbr
export ANDROID_SDK_ROOT=/path/to/Android/Sdk

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily in scope for this PR, but I also encountered the following error when trying to run the Android server

✖ Creating capacitor.config.json in android/app/src/main/assets - failed!
✖ copy android - failed!
[error] Error: ENOENT: no such file or directory, open
        '/Users/ynda/code/github.com/salemlf/hakubun/android/app/src/main/assets/capacitor.config.json'

Creating the assets directory manually before re-running fixed that error, then I got onto the next error, which your suggestion would presumably fix

[error] native-run failed with error

        ERR_SDK_NOT_FOUND: No valid Android SDK root found.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also ran into this issue, so I added a mkdir inside the android-live-reload script in package.json, which fixed the issue for me.
Not sure if the file is meant to have compatibility with windows though, if so, I'll need to think of something else.

Comment thread README.md Outdated

# If any errors occur, set the following environment variables and try again, or run the app using Android Studio
export JAVA_HOME=/path/to/android-studio/jbr
export ANDROID_SDK_ROOT=/path/to/Android/Sdk

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't currently have a ~/Library/Android directory - would installing the Cordova dependencies fix that?

Comment thread README.md Outdated

#### iOS and Android Simulators (with Hot Reload)

Make sure to install Corvoda's required dependencies for [iOS](https://cordova.apache.org/docs/en/11.x/guide/platforms/ios/index.html)/[Android](https://cordova.apache.org/docs/en/11.x/guide/platforms/android/) before running the following commands.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also encountered issues trying to run the iOS and Android servers locally, so I think these extra bits of documentation will be helpful! (That said, I haven't fully tried it yet)

P.s. in case the original wording is kept in some form, there's a typo: Corvoda -> Cordova

@yndajas yndajas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing!

Comment thread README.md Outdated

#### iOS and Android Simulators (with Hot Reload)

Make sure to install Corvoda's required dependencies for [iOS](https://cordova.apache.org/docs/en/11.x/guide/platforms/ios/index.html)/[Android](https://cordova.apache.org/docs/en/11.x/guide/platforms/android/) before running the following commands.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for linking to the v11 docs rather than v12 or whatever version the app currently uses? I'm not familiar with it, but I can't see any reference to a version in the codebase except 10.1.1

Also, you can drop the index.html on the iOS link for consistency 😊

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point, it should really link to the 10.x version instead since that's what's referenced in the variables file you mentioned. Dropping the "index.html" ending makes sense, yeah!

@EricGDevel EricGDevel force-pushed the update_development_readme branch from c73d0eb to ab16fb0 Compare October 14, 2024 12:26
@EricGDevel EricGDevel requested review from salemlf and yndajas October 14, 2024 12:28
@EricGDevel EricGDevel force-pushed the update_development_readme branch from ab16fb0 to d5bc861 Compare October 14, 2024 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants