Skip to content
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

feat(path ios files): [WIP] Support user to specify path for ios/android files #25

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

bduyng
Copy link
Contributor

@bduyng bduyng commented Jul 21, 2017

What: Closing issue #15

Why:

How: generate writeDirPath based on user input

Checklist:

@peggyrayzis
Copy link
Owner

Thanks for your contribution, this is a great start! 😀

I definitely didn't make this clear enough in the issue, but I was picturing the user flow to look something like this:

  • User selects iOS & Android templates
  • We ask the user where they want their iOS files (defaults to ios/)
  • In a separate prompt, we ask the user where they want their Android files (defaults to android/app/src/main/java/com/pkg.name)
  • If the user does not select iOS templates, we don't ask the iOS file location prompt. Same with Android

Since we are now introducing a level of hierarchy into the prompt structure, we will have to change how we're asking the questions because some of the questions depend on a previous answer. Here's an example from the inquirer repo to see how they're handling this.

@bduyng
Copy link
Contributor Author

bduyng commented Jul 25, 2017

@peggyrayzis: You are welcome! I updated the code that follows above guide, please check. I also have a question, I am not sure what directory we need to generate on Android, is it correct if user enter hello/world then we will generate file android/app/src/main/java/com/hello/world/pkg.name?

@peggyrayzis
Copy link
Owner

Hey, sorry for the delay! Recently, I just left my job so I've been taking a break to recharge 😄

pkg.name refers to the name in your package.json file (your app's name if this is an existing RN project). So, if your app name is MyApp, your MainApplication.java will be placed in android/app/src/main/java/com/myapp by default. If the user enters anything in the prompt, then it will override the default & place the bridge module files wherever the user specifies.

Copy link
Owner

@peggyrayzis peggyrayzis left a comment

Choose a reason for hiding this comment

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

Thanks so much for your contribution! It's looking great so far & almost ready to merge. 😀

Also, could you please add a couple tests for this new feature? Test coverage on functionality is something I'm going to be focusing on in the next few weeks because right now I'm only snapshotting the templates.

src/index.js Outdated
type: "input",
name: `${fileType.toLowerCase()}Path`,
message: `What directory should we deliver your ${fileType} files to?`,
default: fileType === "JS" ? "." : fileType.toLowerCase(),
Copy link
Owner

Choose a reason for hiding this comment

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

For the default, we should create an object above with the file types as the keys and the default paths as the values. Then, you can look them up.

You can use the paths that we're currently delivering the files to in RNCB as your defaults.

src/index.js Outdated
@@ -170,17 +193,22 @@ async function createSwiftEnvironment(templateName, templateFolder) {
"ios-swift"
);

if (nativePath !== "ios") {
Copy link
Owner

Choose a reason for hiding this comment

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

nativePath will always be the writeDirPath so I think you can get rid of this if block.

src/index.js Outdated
@@ -189,9 +217,14 @@ async function createObjCEnvironment(templateName, templateFolder) {
"ios-objc"
);

if (nativePath !== "ios") {
Copy link
Owner

Choose a reason for hiding this comment

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

Same comments as the Swift function

@bduyng
Copy link
Contributor Author

bduyng commented Aug 3, 2017

Hey @peggyrayzis. Welcome back! I followed you on Twitter after checking your video at ChainReact and I knew you got a new job. Congratulation!!!

About this PR. Can you give me some hints about how to write tests for this? So I can move faster ;). Thanks!

@peggyrayzis
Copy link
Owner

Thanks! Super excited about my new role at Apollo 🎉

Sure, I'll try to steer you in the right direction. I was thinking of refactoring the tests folder structure anyway so this is perfect timing. I'll add a new file for testing init() and add in some it blocks that you can fill in. Would that be helpful?

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.

2 participants