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

README 2.x update proposal... take 2 #504

Merged
merged 2 commits into from Sep 1, 2018

Conversation

jkantr
Copy link
Collaborator

@jkantr jkantr commented Aug 15, 2018

A continuation of the changes and discussion in #448

This time without broken git history :)

The beginning of a general overhaul of docs to be easier to read, suggest best practices, fix some verbiage, and bring up to date with v2.x

Most suggestions from #448 have either been implemented or added as "comments" in the markdown (TODO 'links')

added "comments" (TODO link references) for changes suggested but not yet implemented for future discussion/prs
    clarified "native stream" to be "native Node streams"
    adjusted all uses of http to https to encourage secure protocol usage
    adjusted whatwg to proper case, WHATWG
    made code block tags consistent as `js` instead of `javascript`
    uppercased all method option values (post vs POST)
    added spec-compliant node to the `response.ok` api section
@codecov-io
Copy link

codecov-io commented Aug 15, 2018

Codecov Report

Merging #504 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #504   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           6      6           
  Lines         503    503           
  Branches      158    158           
=====================================
  Hits          503    503

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09ef40e...2d373bc. Read the comment docs.

@jkantr jkantr added the doc label Aug 15, 2018
@bitinn
Copy link
Collaborator

bitinn commented Aug 16, 2018

Thx @jkantr, I gave this a quick read and it looks good to me.

@jimmywarting @TimothyGu please give this a quick read when you can, I intend to merge it :)

@bitinn
Copy link
Collaborator

bitinn commented Aug 16, 2018

Actually one quick thing:

Can you restructure this code block into a table (just like the default header table below).

I always think people overlooks agent options because they are very much hidden in docs, but it's the one option that workaround many problems.

https://github.com/bitinn/node-fetch/blob/2d373bcb77af75876407fdee64fc572e3a08c11a/README.md#options

@jkantr
Copy link
Collaborator Author

jkantr commented Aug 16, 2018

@bitinn do you mean something like this?
https://gist.github.com/jkantr/a8036d22b6aaec0626ae2340a1aaf1e2

Tables are pretty awkward to work with in .md unfortunately... maybe we could just have a separate sub header about what problems can be solved by using manual agents?

@bitinn
Copy link
Collaborator

bitinn commented Aug 16, 2018

@jkantr yeah, I would move the fetch standard column into a header row, or simply have 2 tables.

If you already got another solution in mind, make a gist, we will take a look. My problem is mainly with (1) the code block isn't great for quick scanning; (2) the explanation on non-standard options could be better.

@jkantr
Copy link
Collaborator Author

jkantr commented Aug 16, 2018

Colspan doesn't really work it seems (can't span the first col, only the ones after) in GFM so this was as close as I can get https://gist.github.com/jkantr/a8036d22b6aaec0626ae2340a1aaf1e2

It may be better just to stuff them in a table and explain each in its own #### section after

@bitinn
Copy link
Collaborator

bitinn commented Aug 17, 2018

It may be better just to stuff them in a table and explain each in its own #### section after

Whichever you see fit, really, I don't have a strong preference on using tables, just the first thing that came to my mind.

@bitinn
Copy link
Collaborator

bitinn commented Aug 31, 2018

@jkantr @jimmywarting @TimothyGu hey guys, last call, I am going to merge this PR, as is, please give OP a thumb up if you feel this is ok. And stop me if you see anything straight up wrong.

@jimmywarting
Copy link
Collaborator

It was good.
You took all other feedback and consideration into account and forged a darn good documentation, like the hole ToC thing!

I didn't read carefully in the end just shimmed thought the rest. Will just say that all looks alright!
So I thought it became to much at the end... there where a lot of things packed into one readme file.
So here are my thoughts: Have a Readme that describes what the fetch api is (and isn't) and then use the github's wiki page

@styfle
Copy link
Contributor

styfle commented Aug 31, 2018

there where a lot of things packed into one readme file

The way we solve this problem with marked is to put the description and a simple getting started snippet in the README (this gets installed millions of times so we keep it small).

The README has a link to GitHub pages which hosts the full documentation (stored in /docs in the repo).

@bitinn
Copy link
Collaborator

bitinn commented Sep 1, 2018

OK I am merging this, any issue we will fix in future PR

@bitinn bitinn merged commit 745a27c into node-fetch:master Sep 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants