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

doctor: improve CocoaPods installation #718

Merged
merged 10 commits into from
Sep 18, 2019

Conversation

lucasbento
Copy link
Member

Summary:

This PR fixes a small problem on the UI of doctor with the CocoaPods installation (tracked at #694), however it introduced a lot of stuff in the code in order to get it done.

The issue basically was that the question was being shown in the middle of the fixes and felt a little bit out-of-place, the idea was to remove the question and do the installation together with a prefix of the method, e.g. CocoaPods (installing with gem).

This turned out to be slightly bigger than expected, in order to remove the question after it's prompted I had to calculate the length of lines the question was using in the terminal.

Before

before

After

after

Test Plan:

  1. sudo gem uninstall cocoapods
  2. node /path/to/cli doctor
  3. Press f
  4. Fall in love

@lucasbento
Copy link
Member Author

lucasbento commented Sep 13, 2019

@thib92: I would appreciate if you could check the code, I did some stuff to get TypeScript work.

Copy link
Contributor

@thib92 thib92 left a comment

Choose a reason for hiding this comment

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

Apart from this one comment, looks good to me 👍

packages/cli/src/tools/installPods.ts Outdated Show resolved Hide resolved
packages/cli/src/commands/doctor/healthchecks/cocoaPods.ts Outdated Show resolved Hide resolved
packages/cli/src/tools/brewInstall.ts Outdated Show resolved Hide resolved
packages/cli/src/tools/installPods.ts Outdated Show resolved Hide resolved
@lucasbento
Copy link
Member Author

@thib92 @thymikee @tido64: all done, please check it out again 🙂

Copy link
Contributor

@thib92 thib92 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Looks good, left some inline feedback

packages/cli/src/commands/doctor/healthchecks/cocoaPods.ts Outdated Show resolved Hide resolved
packages/cli/src/tools/installPods.ts Show resolved Hide resolved
packages/cli/src/tools/installPods.ts Outdated Show resolved Hide resolved
@lucasbento lucasbento requested a review from thymikee September 18, 2019 09:32
@lucasbento
Copy link
Member Author

@thymikee: all fixed 🙂

@thymikee thymikee merged commit 908b34d into master Sep 18, 2019
@lucasbento lucasbento deleted the refactor/separate-cocoapods-installation branch September 18, 2019 10:21
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.

4 participants