Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Update Web Platform Docs #9686

Merged
merged 4 commits into from
Nov 14, 2014
Merged

Conversation

marcelgerber
Copy link
Contributor

Finally got a fix for #6027.
Updated css.json to include the latest data using this Node script.

The only remaining issue (which has existed before): The values are not in any useful order.

Changes (properties added/removed): https://gist.github.com/MarcelGerber/bfdd47849cf58c90a5cd

res.on("end", function () {
console.log("Parsing properties");
propertiesResponse = JSON.parse(propertiesResponse).query.results;
Object.keys(propertiesResponse).forEach(function (propertyName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you already include lodash.unescape, why not depend on lodash itself and use it's forEach method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it doesn't give us much and I included lodash.unescape only as it's pretty lightweight (81KB vs 853KB for lodash-node).

Copy link
Contributor

Choose a reason for hiding this comment

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

I see your point. Probably doesn't matter too much in either case (this script won't be run very often, I would assume.)

@marcelgerber
Copy link
Contributor Author

Forgot to mention: This also updates all URLs from http:// to https:// (while they are currently auto-redirected). Not a big thing, but worth a mention.

@Mark-Simulacrum
Copy link
Contributor

Not sure if this is something to do with your script, but I found that opening Quick Docs on the CSS flex property has an odd heading, that looks like this:

<[[css/properties/flex-grow|flex-grow]]> <[[css/properties/flex-shrink|flex-shrink]]> <[[css/properties/flex-basis|flex-basis]]>

and is supposed to look like this, with each item being a link.

<flex-grow> <flex-shrink> <flex-basis>

@le717
Copy link
Contributor

le717 commented Oct 27, 2014

:D

@marcelgerber
Copy link
Contributor Author

Oh damn. At least, I don't need lodash.unescape any more.
@Mark-Simulacrum I've fixed the bug you mentioned, and probably (hopefully) thousands more.

@Mark-Simulacrum
Copy link
Contributor

Found another issue with the Inline Docs for the alignment-adjust property. The values of the property are empty in the downloaded file; here is the JSON for it:

"css\/properties\/alignment-adjust": {
            "SUMMARY": "",
            "INITIALVALUE": "",
            "ID": "css\/properties\/alignment-adjust",
            "ANIMATABLE": false,
            "COMPUTEDVALUE": "",
            "URL": "http:\/\/docs.webplatform.org\/wiki\/css\/properties\/alignment-adjust",
            "STATUS": "",
            "VALUES": [

            ]
        }

The Docs display just the title and nothing else, I would expect them to not appear at all.

@marcelgerber
Copy link
Contributor Author

@Mark-Simulacrum Your JSON snippet is part of the old file ;) But you're right, there are such cases with the updated file as well, like for css/properties/behavior. I did this on purpose as call to map() in InlineDocsViewer.js expects VALUES to be an array, but it's probably better to just add a condition there.

PS: Thanks for taking a look!

@Mark-Simulacrum
Copy link
Contributor

Is it intentional that the (new) JSON file has a space after every TITLE string? (e.g."TITLE": "justify ").

@Mark-Simulacrum
Copy link
Contributor

Actually, upon further inspection, every string in the file ends with a space. Is this something in update-docs.js?

@marcelgerber
Copy link
Contributor Author

It's something instaview (the Wikitext parser) does. But I have to admit, I actually like it as it somehow makes Brackets way faster when viewing that file, probably due to Word Wrap being less performances heavy.

@Mark-Simulacrum
Copy link
Contributor

Here is a few more bugs, will update this list as I go along to prevent posting too many comments:

  • Many (if not all) summaries/descriptions start with <p> but the paragraph tag is not closed.
  • animation-fill-mode property, see forwards value section; rendering has {| class="mw-datatable os-suggest-results filehistory", seems out of place.

@marcelgerber
Copy link
Contributor Author

@Mark-Simulacrum A closing <p> tag is not required (see http://stackoverflow.com/questions/8460993/p-end-tag-p-is-not-needed-in-html), and InstaView omits it therefore.
The table bug is fixed, but our tables could use some design love.

@marcelgerber
Copy link
Contributor Author

I just removed the extra step required to convert the JSON data to a usable format.

@@ -49,19 +49,15 @@ define(function (require, exports, module) {

/**
* @param {!string} cssPropName
* @param {!{SUMMARY:string, URL:string, VALUES:Array.<{TITLE:string, DESCRIPTION:string}>}} cssPropDetails
* @param {!{SUMMARY:string, URL:string, VALUES:?Array.<{value:string, description:string}>}} cssPropDetails
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the JSDoc gurus: Is this (to be exact, the VALUES:?Array) valid?

@redmunds
Copy link
Contributor

@marcelgerber Thanks for taking a look at this. Those docs are way out-of-date.

It looks like update-docs.js is a nodejs module with a dependency of instaview that updates the css.json file -- is that right? If so, that's an awesome contribution!

At the very least, the README.md file should mention that and also have a recipe for installing and running it. Since this code is never run by Brackets and introduces a new dependency, I'm thinking it should be in a separate repo outside of Brackets, but I'm open to other opinions. Moving it to a separate repo would also make it much easier to get the changes into css.json into Brackets.

@marcelgerber
Copy link
Contributor Author

Yep, the ‘update-docs.js‘ is a node script to update the css.json file using the WebPlatformDocs/MediaWiki API.
I'm not quite sure whether it belongs in the brackets repo or not - it's basically a maintenance script, and other maintenance helpers like set-release are in the brackets repo as well.
Talking of set-release, I could imagine a similar workflow with update-docs as well - running it every time a new Release is set would be great, to keep the docs up to date.

@redmunds
Copy link
Contributor

@marcelgerber I didn't think about the Tools folder -- I was thinking about the Apify nodejs app for creating Brackets API docs which is at: https://github.com/jbalsas/apify

@marcelgerber
Copy link
Contributor Author

@redmunds I've completely removed the Node script and took it over to https://github.com/MarcelGerber/update-wpd-docs.
I've created a README with update instructions as well.

@marcelgerber marcelgerber changed the title Update Web Platform Docs & Provide Update Script Update Web Platform Docs Nov 13, 2014
@redmunds
Copy link
Contributor

I am seeing a WebPlatformDocs unit test fail:

should process all anchor tags

Error: Expected 1 to be 9.
    at new jasmine.ExpectationResult (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:114:32)
    at null.toBe (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1235:29)
    at null.<anonymous> (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/src/extensions/default/WebPlatformDocs/unittests.js:210:39)
    at jasmine.Block.execute (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1064:17)
    at jasmine.Queue.next_ (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2096:31)
    at jasmine.Queue.start (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2049:8)
    at jasmine.Spec.execute (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2376:14)
    at jasmine.Queue.next_ (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2096:31)
    at jasmine.Queue.start (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2049:8)
    at jasmine.Suite.execute (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2521:14)
    at jasmine.Queue.next_ (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2096:31)

@redmunds
Copy link
Contributor

-ms-flex-pack is the only vendor-specific property -- should it be removed?

@marcelgerber
Copy link
Contributor Author

@redmunds Fixed the unit tests and removed the vendor-prefixed property.

@redmunds
Copy link
Contributor

Thanks. Merging.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants