-
Notifications
You must be signed in to change notification settings - Fork 12
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
Updates instructions to encourage SSH and fixes incorrect images/instructions #18
Conversation
use the modern UI with 60,000 stars later ⭐)
fixes actual UX problem in that the button text changed
…cated HTTPS URL encouraged
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.
self-review
noticed the images are notably smaller. I could retake them on a retina screen if needed (I did all this quick on my Windows machine after the kiddos went to bed)
@@ -6,7 +6,7 @@ | |||
<title>Getting Started With Node.js Core Development</title> | |||
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/normalize/3.0.3/normalize.min.css" type="text/css"> | |||
<link href="https://fonts.googleapis.com/css?family=Baloo+Tamma%7COpen+Sans" rel="stylesheet"> | |||
<link rel="stylesheet" href="../style.css" type="text/css"> |
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.
I could not render locally without this change. Perhaps I am testing it differently than expected.
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.
Yeah, this change is correct. I goofed this up earlier this week. It doesn't matter on the server because ..
is still the root webdocs directory, but it means local testing gets messed up. Thanks for fixing it.
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.
I'll land this now and someone (you, me, someone else) can replace the images in a subsequent PR if they are too small on the website.
Thanks for the improvements!!! ❤️ |
Today during Open Source Day I was fortunate enough to help 14 people interact with the Node project for the first time. I went through the same setup as them a week or so earlier, using this document. I glossed over much of it, already knowing or assuming things along the way. But attendees today took the advice here much more literally, and I had to help them a number of times.
I wish I'd seen this all before the event, but better late than never!
Incorrect "Clone or Download" Instruction and Image
This image and button text is from an older version of GitHub
I've updated it to be accurate (and made it more clear it's from a fork). Updating the images also has the benefit of refreshing the numbers and UI chrome a bit.
Instructions encourage HTTPS cloning (implied by the HTTPS upstream), which is deprecated
Per this announcement and follow up, GitHub removed authenticated git actions a bit ago. We don't notice this right away because
git clone
is unauthenticated.Here's a complete reproduction:
I've updated the docs with a suggestion to use an SSH key. This seems like a better choice than a personal access token, but I could be swayed.