-
Notifications
You must be signed in to change notification settings - Fork 904
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: show required versions #763
Conversation
I can see how that looks nice with colors, but please notice that this command will be ran in case of issues, likely by inexperienced RN users. We need to be more descriptive than showing an arrow. I'd rather like to give it a human-readable text like "We found vX, however we need vY".
I'm not a fan of this change. Initially when I read the "Watchman" and its description below, I was confused where it belongs. Let's either keep it in the same line (why moving it? it fits nicely even with version), or indent it one level or do something else. How about this UI:
|
) => Promise<{version?: string; needsToBeFixed: boolean | string}>; | ||
) => Promise<{ | ||
version?: string; | ||
versionRange?: string; |
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.
shouldn't we require versions and ranges from all health checks?
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.
Some of them are not based on comparing versions, just checking if they are there or not (e.g. ios-deploy
and cocoapods
).
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.
Thank you, code LGTM! Doctor starts to shape up :D. Let's iterate on the UI, to be a bit more approachable for beginners, maybe by structuring the output in a easy-to-follow important stuff way and being descriptive in messaging
@thymikee: cool, thanks for the review. The approach you suggested looks good, I'll see what I can do! |
@thymikee: what do you think about this? A few points related to your proposal:
|
Looks good! |
@thymikee: pushed :) |
@@ -47,11 +47,15 @@ export default { | |||
} catch {} | |||
} | |||
|
|||
const version = sdks === 'Not Found' ? sdks : sdks['Build Tools'][0]; |
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.
What if I forgot to install build-tools
when using sdkmanager
manually? Here will throw an Cannot read property '0' of undefined
error, instead of telling me to install build-tools
through sdymanager "build-tools;29.0.0"
.
Summary:
This PR is related to #694 and it:
to fix all issues
to `to try to fix all issues;description
content below the health check name rather than on the right side.Preview
Test Plan:
Run
doctor
.