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

Edit and proofread CONTRIBUTING.md #435

Merged
merged 27 commits into from
Apr 22, 2020
Merged

Edit and proofread CONTRIBUTING.md #435

merged 27 commits into from
Apr 22, 2020

Conversation

jtoar
Copy link
Contributor

@jtoar jtoar commented Apr 17, 2020

This is for reference for #434.

This PR includes changes that I think would be immediate improvements on the document as it stands. It adds a short welcome message, and edits/proofreads CONTRIBUTING.md generally, but shies away from making any major organizational changes.

@jtoar jtoar changed the title Edit and Proofread CONTRIBUTING.md Edit and proofread CONTRIBUTING.md Apr 17, 2020
@thedavidprice
Copy link
Contributor

This is good progress. Let's see if there are any learnings/updates to apply from the thread in #429

Also, I'd be game to try the Google Docs to PR workflow on this one. I have comments I want to add here, but never feels easy as it could be using code review tools for Markdown. Wanna give the Docs process I described in the Forum DM a shot?

@peterp
Copy link
Contributor

peterp commented Apr 17, 2020

This is so great, thank you @jtoar <3!

@jtoar
Copy link
Contributor Author

jtoar commented Apr 17, 2020

@thedavidprice Let's do it--I'll close this out and open an issue instead? And @peterp of course!

@jtoar
Copy link
Contributor Author

jtoar commented Apr 17, 2020

@thedavidprice Opened #439.

@thedavidprice
Copy link
Contributor

thedavidprice commented Apr 17, 2020

Consolidating notes:

@jtoar
Copy link
Contributor Author

jtoar commented Apr 22, 2020

Going to request a final review from @thedavidprice (sorry it took much so long). All the commands work (on my machine), as written, in the order they're written.

I applied most of the edits in the google docs. A few things that are different:

  • Table of content section names are the same as the section. So Local Package Registry Emulation is in the table of contents instead of Emulate NPM.
  • The actual rwt command is copy:watch, so I renamed the section Copy and Watch (instead of Watch and Copy)
  • I don't have instructions for creating an environment variable on Windows so the section's just not there.

Going to reference an issue from the @redwoodjs/redwoodjs.com repo: redwoodjs/redwoodjs.com#15. The code blocks for explaining how ./tasks/publish-local works might confuse cursory readers. I.e., this isn't actually meant to be run:

npm unpublish --tag dev --registry http://localhost:4873/ --force
npm publish --tag dev --registry http://localhost:4873/ --force

As of now I've just left it there. Definitely something for the future though.

@peterp
Copy link
Contributor

peterp commented Apr 22, 2020

The code blocks for explaining how ./tasks/publish-local works might confuse cursory readers.

That's a very good point!

@thedavidprice
Copy link
Contributor

@jtoar Apologies but I'm in a time crunch and made a mess of the workflow to commit back to your original branch. Could you see my edits here in this repo? https://github.com/thedavidprice/contributing/blob/master/CONTRIBUTING.md

Pull in all you can/want to your branch, and then we'll get this merged before a release. Thanks again!

Dominic added 3 commits April 22, 2020 11:20
Some of the instructions for how commands worked could've confused
cursory readers.
Now that they're in asides (i.e. prefixed by >) that should be fixed.
@jtoar
Copy link
Contributor Author

jtoar commented Apr 22, 2020

@thedavidprice Double checking now but think I made every change in the doc you linked here. So should be ready to merge!

Copy link
Contributor

@thedavidprice thedavidprice left a comment

Choose a reason for hiding this comment

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

@jtoar You're a champion. And apologies again for making the workflow a pain. We'll figure this out.

Two quick changes for you to accept (one a redundant sentence and the other a new line).

🚀

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
jtoar and others added 2 commits April 22, 2020 12:14
Co-Authored-By: David Price <thedavid@thedavidprice.com>
Co-Authored-By: David Price <thedavid@thedavidprice.com>
@thedavidprice thedavidprice merged commit e2ff788 into redwoodjs:master Apr 22, 2020
@thedavidprice thedavidprice added this to the next-release milestone Apr 22, 2020
@jtoar
Copy link
Contributor Author

jtoar commented Apr 22, 2020

@thedavidprice applied! No worries, I'm learning a lot just from the back-and-forths; we'll have six more chances coming up in the six READMEs for #461 😅

@thedavidprice
Copy link
Contributor

Indeed!

Well, just know your work isn’t taken for granted. We’ll get this smoothed out going forward.

@jtoar
Copy link
Contributor Author

jtoar commented Apr 22, 2020

👍 No worries at all you and the team are a pleasure to work with!

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.

3 participants