Add "components" docs #8337

wpscholar · 2018-07-31T20:50:36Z

Description

Autogenerate components documentation. Pulls in all readme files from https://github.com/WordPress/gutenberg/tree/master/packages/components and automatically adds them to the Gutenberg docs. Implemented lint-staged and husky pre-commit hook to automatically run a build of the docs in the event that a file has changed that will likely result in a need for rebuilding the docs. Additionally, I automated validation of package.json files and linting of JS files that have changed.

Fixes #4952

How has this been tested?

I've manually tested the scripts and can see the appropriate entries added to the manifest.json file. I've also tested the lint-staged pre-commit hooks to ensure that they work properly.

Types of changes

New feature

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

…hildren.

…file, any change to the docs tooling, and package files that would likely require rebuilding the docs. Also automated running of js and package.json linting. All occurs on a pre-commit hook.

gziolo · 2018-08-01T05:28:03Z

Awesome work. I will try to review this PR later this week 💯

By the way, @mmtr is working on improvements for the documentation of components. Both #8267 and #8338 should be good PRs to test together with those changes.

gziolo · 2018-08-01T05:31:51Z

package.json

+	},
+	"lint-staged": {
+		"packages/*/package.json": [
+			"wp-scripts lint-pkg-json"


We should rather use npm run lint-pkg-json in here to ensure it always uses the universal command.

@gziolo If we do that, then it will run for all of the package.json files and not just the ones that have changed.

Right, I didn't think about it. Good call 👍

gziolo · 2018-08-01T05:32:18Z

package.json

+			"wp-scripts lint-pkg-json"
+		],
+		"*.js": [
+			"eslint"


We should rather use npm run lint-js in here to ensure it always uses the universal command.

@gziolo If we do that, then it will run for all of the JavaScript files and not just the ones that have changed. It is very slow when scanning everything.

Essentially the lint staged tool will automatically pass the files that have changed to the command. Because the npm run lint-js script passes . as the path, it will cause it to run eslint against all .js files in the project. By just using the eslint command here, only the files that have changed are linted. Besides, we don't want linting problems caused by someone else's commit to prevent our own.

Unless you use git add -p for partially staging files, if you do stage files in this manner lint-staged is unable to detect the difference.

See lint-staged/lint-staged#62

Aside: we need wp-scripts lint-js anyways, that would be a way to provide one interface in the long run.

gziolo · 2018-08-01T05:33:53Z

package.json

+		"*.js": [
+			"eslint"
+		],
+		"{docs/{root-manifest.json,tool/*.js},packages/{*/src/{actions,selectors}.js,components/src/*/README.md}}": [


I bet this should include all type of README.md files located inside packages folder.
We should also update when new packages/*/README.md is added.

Is it overkill to run this on every commit? How long does this task take to run?

I think it only generates manifest.json file. It's super fast.

@ntwb It only runs if a file has changed that matches the patterns listed here. It is also super fast.

@gziolo Agreed.

…eration.

…en `Components Package Reference`

wpscholar · 2018-08-01T18:48:33Z

@gziolo I've pushed an update to the PR... would love to get a final review from you. Should I also wait on the other reviewers you added before we merge?

youknowriad · 2018-08-01T18:53:34Z

docs/tool/config.js

@@ -3,15 +3,17 @@
 */


This is meant to be a "config" file, I'd personally move all the logic to generate the manifest to docs/tool/manifest.js and leave here only the parts that are "configurable"

I guess the line is not always easy to draw. We should probably rethink this config globally though. So don't take this comment as a big blocker.

That is what I was going to do initially, but that file exports a single function that is very specific to the logic for the packages and data docs. I considered exporting an object from that file which contained more than one function but went the path of least resistance. I'd be happy to make some changes if you have a specific recommendation.

Since the function is already doing two things, it can do three :) and we can split it into three functions if necessary.

I'd only keep 'packages/components/src' as a config variable. Something like: componentsSource or something like that, and just move everything else to manifest.js.

Also it looks like the baseRepoUrl is already being used in that file (hardcoded), so it might be good to use this variable consistently.

Let's address this feedback and 🚢

I really like the automation proposed in this PR 💯

…nerated manifest from config.js. Regenerated docs.

wpscholar · 2018-08-07T19:56:49Z

@youknowriad If you can give this a final review, I'd appreciate it. Thanks!

youknowriad

Nice work

youknowriad · 2018-08-08T08:32:44Z

docs/tool/manifest.js

-
-	return packagesManifest.concat( dataManifest );
+module.exports = {
+	getPackageManifest: function( packagesConfig ) {


Minor; I would have moved those functions above (and added JSDocs) and only export the getManifestconcatenating the three manifests.

gziolo · 2018-08-08T09:23:36Z

I tried to resolve conflicts using GitHub UI: c2287a9. Not sure if it is perfect though :)

gziolo · 2018-08-08T10:47:05Z

Applied two more commits to include the latest generated version of files.

gziolo · 2018-08-08T11:08:10Z

Another merge conflict resolved. I will merge it as soon as Travis is happy.

Great work @wpscholar. Let's iterate on it in the follow-up PRs.

I think we should add support in pre-commit hook for stylelint which was added today by @noisysocks.

wpscholar added 4 commits July 31, 2018 15:46

Auto-generate components documentation manifest

f4e76a8

Ensure that the packages root doc is defined immediately before its c…

45d64fe

…hildren.

Ran npm run docs:build

6e6abd8

Automated build of docs based on changes to the root-manifest.json …

4d95a66

…file, any change to the docs tooling, and package files that would likely require rebuilding the docs. Also automated running of js and package.json linting. All occurs on a pre-commit hook.

wpscholar added the [Type] Developer Documentation Documentation for developers label Jul 31, 2018

wpscholar self-assigned this Jul 31, 2018

wpscholar requested a review from gziolo July 31, 2018 20:50

gziolo requested review from pento, tofumatt, youknowriad and a team August 1, 2018 05:28

gziolo added the [Type] Build Tooling Issues or PRs related to build tooling label Aug 1, 2018

gziolo requested a review from ntwb August 1, 2018 05:28

gziolo reviewed Aug 1, 2018

View reviewed changes

gziolo added this to the 3.5 milestone Aug 1, 2018

ntwb approved these changes Aug 1, 2018

View reviewed changes

wpscholar added 5 commits August 1, 2018 10:32

Ensure that packages/*/README.md file changes also trigger docs gen…

c682113

…eration.

Rename Components parent title to Components Package Reference

25dc1a2

Ensure that Packages shows first, then Data Package Reference, th…

e31f561

…en `Components Package Reference`

Automatically ran npm run docs:build

1264078

Merge branch 'master' into add/components-docs

7bb46f3

youknowriad reviewed Aug 1, 2018

View reviewed changes

wpscholar added 2 commits August 7, 2018 15:15

Merge branch 'master' into add/components-docs

cbc846f

Split up manifest.js into separate functions and removed code that ge…

34db1da

…nerated manifest from config.js. Regenerated docs.

youknowriad approved these changes Aug 8, 2018

View reviewed changes

Merge branch 'master' into add/components-docs

c2287a9

gziolo added 3 commits August 8, 2018 11:36

Update package-lock.json

260cb62

Update data-core.md

9ee1205

Update manifest.json

2ab9aa0

gziolo and others added 3 commits August 8, 2018 13:02

Update data-core-editor.md

e4c90e6

Update manifest.json

e2814c2

Merge branch 'master' into add/components-docs

7897de5

gziolo merged commit b1196b8 into WordPress:master Aug 8, 2018

wpscholar mentioned this pull request Aug 8, 2018

Update lint-staged and docs/manifest.js #8723

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "components" docs #8337

Add "components" docs #8337

wpscholar commented Jul 31, 2018 •

edited by gziolo

Loading

gziolo commented Aug 1, 2018

gziolo Aug 1, 2018

wpscholar Aug 1, 2018

gziolo Aug 2, 2018

gziolo Aug 1, 2018

wpscholar Aug 1, 2018

wpscholar Aug 1, 2018

ntwb Aug 1, 2018

gziolo Aug 2, 2018

gziolo Aug 1, 2018

ntwb Aug 1, 2018

gziolo Aug 1, 2018

wpscholar Aug 1, 2018

wpscholar commented Aug 1, 2018

youknowriad Aug 1, 2018

youknowriad Aug 1, 2018

wpscholar Aug 1, 2018

youknowriad Aug 1, 2018

gziolo Aug 2, 2018

wpscholar commented Aug 7, 2018

youknowriad left a comment

youknowriad Aug 8, 2018

gziolo commented Aug 8, 2018

gziolo commented Aug 8, 2018

gziolo commented Aug 8, 2018

Add "components" docs #8337

Add "components" docs #8337

Conversation

wpscholar commented Jul 31, 2018 • edited by gziolo Loading

Description

How has this been tested?

Types of changes

Checklist:

gziolo commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpscholar commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpscholar commented Aug 7, 2018

youknowriad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Aug 8, 2018

gziolo commented Aug 8, 2018

gziolo commented Aug 8, 2018

wpscholar commented Jul 31, 2018 •

edited by gziolo

Loading