Repository navigation
Conversation
Add blurb to N-API documentation indicating that we recommend the use of badges Fixes: #22796
cjihrig
left a comment
There was a problem hiding this comment.
I won't block this, but I don't think it belongs in the docs.
|
@cjihrig just wondering if you have alternate suggestions where you think it would make more sense to publicize this kind of info? |
|
@gabrielschulhof FYI in case you have not seen this yet. |
Made sure text has a maximum of 80 characters per line Added space after heading Deleted extra line at end of blurb
|
@ddlan Did you close it intentionally? |
|
@vsemozhetbyt Yes, would it be better for me to leave it open? |
|
Well, I see two approvals and no blocking objections, so this can be landed if I am not mistaken) |
Sorry, I missed that response. I don't have an alternative suggestion, I just don't feel like it belongs in the official Node docs. Shields.io and other services offer a bunch of different badges, but we don't suggest using any other ones. And, why do I need a badge? Why not just write it out in the project's README. Again, I don't feel strongly enough to block it, but it seems odd. |
|
@cjihrig we're taking this step to drive adoption of N-API. We do so after having received feedback in support in nodejs/admin#221. |
addaleax
left a comment
There was a problem hiding this comment.
Please don’t include external resources in our official API docs, since it leaks usage data to the image hoster.
That being said, I agree with @cjihrig in that this is not really something that should go into our official documentation.
|
@addaleax In that case, what's the best way to add the images to the documentation? |
|
I suppose that if we do add this, downloading them to |
The badge images were originally hosted on an external website but has been added to the node repository as not to leak usage data.
|  | ||
|  | ||
|  | ||
|  |
There was a problem hiding this comment.
Have you verified that this works in the make doc output? I think most of our assets are included via assets/, not some ../-prefixed path
|
@ddlan could you please redo this PR against the doc branch at abi-stable-node, and add the blurb to README.md in that branch instead? That might be a better place for it. |
|
It seems like this has not reached consensus and conversation has stalled. We should either close it or escalate to the TSC for a decision. I'm going to close it, but feel free to leave a comment if you feel that escalation to the TSC is the better approach. (I'm personally of the opinion that this is somewhat out of place in our docs and is better suited for another repo, as suggested by @gabrielschulhof.) |
|
@ddlan Thanks for the pull request. I hope the experience was not discouraging and I hope you will continue to submit pull requests for Node.js! |
Add blurb to N-API documentation indicating that we recommend the use of badges. Fixes: nodejs/node#22796 Re: nodejs/node#22801
Add blurb to N-API documentation indicating that we recommend the use of badges. Fixes: nodejs/node#22796 Fixes: nodejs#333 Re: nodejs/node#22801
Add blurb to N-API documentation indicating that we recommend the use of badges. PR-URL: #357 Fixes: nodejs/node#22796 Fixes: #333 Re: nodejs/node#22801 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Add blurb to N-API documentation indicating that we recommend the use of badges
Fixes: #22796
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes