intel / zephyr.js

JavaScript* Runtime for Zephyr* OS
Other
180 stars 65 forks source link

Update WebIDL and modify documentation to fit new "node.js-like" style, Part 2 #1893

Closed t-harvey closed 6 years ago

t-harvey commented 6 years ago

Folks, This is the actual pull request for updating the documentation to: 1) follow the style of node.js and 2) correct the WebIDL.

I tried to incorporate the advice/corrections from the original test pull request (#1800).

As with the test pull request, I've limited myself to fixing grammatical errors and the actual WebIDL, but the documentation could definitely do with a thorough going over for technical detail as well.

That said, I've run all of the updated WebIDL through my own parser, and it all builds correctly. I wrote an automated tester for this, but I'm largely a newbie, so I don't know where I can put it and how to get it to run as part of the automated testing; any advice, pointers, etc. would be very useful.

Thanks, Tim

jimmy-huang commented 6 years ago

@t-harvey looks good to me, thank you for sending this patch and help us fixing up the web-idls, definilte a lot better than the previous one, we are okay with merging this patch as is, but I would like to know if there's a reason to use <p> for all these new lines? Is this like a editor-generated styles that you were using or this is the standard practice for webidls?

t-harvey commented 6 years ago

Jimmy, In the "test" pull request for updating the docs, one of the reviewers wanted the WebIDL put into a separate file and just linked to -- but I didn't think this worked very well, so I decided I'd try using the markdown feature called "details", which, as you can see, lets you hide away details until you need them. Unfortunately, the text inside "details" is interpreted as html rather than as markdown, so we have to use html formatting there -- and I soon learned that without using the <p>'s, all of the indentation goes away. Thus, we're stuck with some ugly text that displays nicely. If you've got a better method (b/c I'm not an html expert!), I'm all ears...but as I said, I've got a script that rips out the WebIDL for testing, and it was trivial to get it to ignore <p>, so it's easily worked around, and you and I are the only ones who'll ever see the awkwardness of the text... :-)

grgustaf commented 6 years ago

Yeah, I looked through it too and looks good. Since it's all in documentation there should be no problem with just merging it. :)

@t-harvey are you sure you still needed the <p> tags after you started using <pre>? I think that takes all your newlines literally. So I suspect you could remove all those. Also don't need the double blank lines in some places.

grgustaf commented 6 years ago

LGTM +1