-
Notifications
You must be signed in to change notification settings - Fork 63
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
base: master
Are you sure you want to change the base?
feat(path ios files): [WIP] Support user to specify path for ios/android files #25
Conversation
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:
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 |
@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 |
Hey, sorry for the delay! Recently, I just left my job so I've been taking a break to recharge 😄
|
There was a problem hiding this 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(), |
There was a problem hiding this comment.
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") { |
There was a problem hiding this comment.
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") { |
There was a problem hiding this comment.
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
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! |
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 |
What: Closing issue #15
Why:
How: generate
writeDirPath
based on user inputChecklist: