DevDocs: Generate a table for displaying proptypes #10342

withinboredom · 2016-12-30T19:09:25Z

Generates a table of proptypes when viewing a component by itself in the DevDocs.

#9919 added a json file to the build that uses react/react-docgen to parse our components and extract the propTypes for each of them. This happens at build time. A later PR may allow this to run similar to how our CSS files are built while developing.

This PR requires that file directly, and doesn't require a server. This has two benefits:

It keeps the code simple
It doesn't require a running server (offline mode -- as seen above)

A different PR can move the file somewhere in the client path. I'm not particularly happy about a relative path in this code. However, I feel as though that should be separate and not done in this PR.

The styling of this follows @aduth's state selector PR (#9433) style. I've tagged this for design review. I originally experimented with using flex-box instead of a table, but the complexity of the dom and CSS exploded on me. It would be a good future task, though I imagine most developers would be on a desktop device writing code. I could be wrong 😉 ...

There are some interesting possibilities that could come in a future PR near you:

Instead of using the component's JSDoc for a description, use the readme for that component.
@retrofox has a really awesome PR (Devdocs: add isolate mode #8482) that could really bring this to life.
Show an editable example under in the "Use It" box, similar to @enejb's Add component playground to devdocs #8284 or my DevDocs: Auto Proptype Documentation for Components and Blocks #9289.
Build the json file if a component changes, similar to how our CSS is currently built.

This work is based on #9289, #9919. There's a fix in #10343 to get some missing components into the index.

View live: https://calypso.live/devdocs/blocks?branch=add/proptypes-table

folletto · 2017-01-04T13:27:40Z

I think design wise it's good for this iteration. I agree it might require tweaking, but it's already good enough, and we can iterate on that once it's in.

The only frustrating bit is that going back to "All Blocks" loses the scroll position in the list, which makes navigating a few blocks fairly frustrating. Might not be something to be done in this PR specifically, but I think it's important to address that.

withinboredom · 2017-01-04T15:20:10Z

@folletto

The only frustrating bit is that going back to "All Blocks" loses the scroll position in the list, which makes navigating a few blocks fairly frustrating

Yes, that is really annoying... hopefully I'll figure out a fix soon because it drives me bananas.

ehg · 2017-01-04T22:18:19Z

I noticed after cherry-picking #10343, that inline comments aren't being displayed for propTypes documented with inline. e.g. http://calypso.localhost:3000/devdocs/blocks/theme doesn't display a description, though it documents its propTypes: https://github.com/Automattic/wp-calypso/blob/master/client/components/theme/index.jsx#L22

Any idea how we'd fix this?

ehg

Looking good so far. I left you some initial pass comments :)

ehg · 2017-01-04T22:23:24Z

client/blocks/props-viewer/index.jsx

+		 * @param {string} propName The name of the prop
+		 * @return {ReactElement} The row
+		 */
+		const row = ( propName ) => {


Could we extract functions that render stuff into components?

Ah yes ... I think I was being a little too paranoid about keeping the changes under a couple hundred lines ;)

ehg · 2017-01-04T22:28:05Z

client/blocks/props-viewer/index.jsx

+		 * @param {string} rightName The prop name of the right side
+		 * @return {number} Which side wins
+		 */
+		const sortProps = ( leftName, rightName ) => {


This feels like it should live outside render() — it also makes me think that we're creating a new function per render?

withinboredom · 2017-01-05T06:05:53Z

@ehg said:

inline comments aren't being displayed for propTypes

Looks like it only supports docblocks.

It appears like there was commit in react-docgen near where I'd need to update it to support this as it just strips the comments, if I'm reading it correctly. I don't know how the maintainers feel about it ... so I opened an issue. Short of that, I think a codemod might would work... I know I've thought about a codemod to move the readme to the component at some point in the future, but I don't think I've brought it up, just thought about it.

ehg · 2017-01-05T14:01:28Z

client/blocks/props-viewer/index.jsx

+	}
+}
+
+PropsViewer.propTypes = {


These could be put in the class as a static property :]

ehg · 2017-01-10T12:29:37Z

client/blocks/props-viewer/index.jsx

+/**
+ * External Dependencies
+ */
+


No space needed here.

ehg · 2017-01-10T12:30:38Z

client/blocks/props-viewer/index.jsx

+		);
+	}
+
+	static propTypes = {


Any benefits to having these at the bottom, rather than the top of the class?

I normally see them outside the class at the bottom of the file. I put them at the bottom so that someone would easily find them out of habit, without searching.

Yeah, I think we only do that because we need Component assigned, before we modify Component.propTypes. With createClass, we'd put them at the top.

ehg · 2017-01-10T12:44:02Z

Makefile

@@ -164,7 +164,7 @@ build-dll: node_modules
 	@mkdir -p build
 	@CALYPSO_ENV=$(CALYPSO_ENV) $(NODE_BIN)/webpack --display-error-details --config webpack-dll.config.js

-build-server: install
+build-server: install server/devdocs/proptypes-index.json


What environments will this actually run in? Do we actually need to generate a full proptypes index in the test environment?

I realized I forgot to commit my tests, and the build failed without that file existing. I'd like to chat about it in slack if you don't mind. I don't like it.

Did we converge on a solution to this? :)

Yeah, needs mock and this reverted. Still working on it, taking a bit longer than I anticipated.

ehg · 2017-01-10T12:49:40Z

client/blocks/props-viewer/index.jsx

+	 * @param {object} component The component to render for
+	 * @return {ReactComponent|null} The table or nothing
+	 */
+	renderTable = ( component ) => {


Why not use a regular, unbound, function here?

ehg · 2017-01-10T12:51:15Z

client/blocks/props-viewer/test/index.jsx

+
+const components = require( '../../../../server/devdocs/proptypes-index.json' );
+
+const findRealComponent = ( slug ) => {


Why are we duplicating code here?

My personal strategy with duplicate code is to always duplicate once before refactoring for reuse. I do need to address this.

ehg · 2017-01-10T12:53:04Z

client/blocks/props-viewer/index.jsx

+			return null; //todo: explain why this table is missing
+		}
+
+		return (


This still feels a bit long, and unreadable to me.

It does feel a bit long, but it's mostly just markup. Lines 155 - 160 could probably become more readable.

Could we split this into a few subcomponents? Ideally it'd be nice if we don't fork render logic into separate methods (often that's a sign it should be split to separate components).

ehg · 2017-01-10T12:54:02Z

client/blocks/props-viewer/test/index.jsx

+		it( 'can render itself', () => {
+			const example = ( <div>Example goes here</div> );
+			const component = ( <PropsViewer component={ 'props-viewer' } example={ example } /> );
+			const spection = findRealComponent( 'props-viewer' )[ 0 ];


What's a spection?

Spection comes from Latin specere: to look.
It technically isn't a real word, but one I made up ;)

Hah, I guess I'd like something a little more human readable though. Makes me feel less of a caudex :P

withinboredom · 2017-01-19T16:21:49Z

I moved the build of the json file to only be when make run or make dashboard happens instead of on every build. That would mean it won't show on calypso.live or stage ... so, I'm going to have to find a happy place it should be built at. :grumble: ...

withinboredom · 2017-01-25T13:08:09Z

Makefile

@@ -172,7 +172,7 @@ build: install build-$(CALYPSO_ENV)

 build-css: public/style.css public/style-rtl.css public/style-debug.css public/editor.css

-build-development: server/devdocs/proptypes-index.json server/devdocs/components-usage-stats.json build-server build-dll $(CLIENT_CONFIG_FILE) server/devdocs/search-index.js build-css
+build-development: server/devdocs/components-usage-stats.json build-server build-dll $(CLIENT_CONFIG_FILE) server/devdocs/search-index.js build-css

 build-wpcalypso: server/devdocs/proptypes-index.json server/devdocs/components-usage-stats.json build-server build-dll $(CLIENT_CONFIG_FILE) server/devdocs/search-index.js build-css


Now also builds with wpcalypso

ehg · 2017-01-27T21:03:59Z

Makefile

@@ -91,10 +91,10 @@ welcome:
 install: node_modules

 # Simply running `make run` will spawn the Node.js server instance.
-run: welcome githooks install build
+run: welcome githooks install build server/devdocs/proptypes-index.json


Superfluous due to https://github.com/Automattic/wp-calypso/pull/10342/files#diff-b67911656ef5d18c4ae36cb6741b7965R171 ?

ehg · 2017-01-27T21:04:36Z

Makefile

 	@$(NODE) build/bundle-$(CALYPSO_ENV).js

-dashboard: install
+dashboard: install server/devdocs/proptypes-index.json


Also superfluous :)

returning to this since I have a few days and not sure if any issues remain from this review.

withinboredom · 2017-10-03T15:08:25Z

Closing because I suck at making time for this.

DanReyLop · 2017-10-22T21:02:23Z

In #19064 I first made the generate-proptypes-index task a bit faster in cee40bd (from 30s to about 25s) but then removed it entirely because it's not being used. So if we resurrect this PR, we'll need to add it again, and I recommend using the optimized version. For that, re-add this and these lines to package.json, and restore this file.

withinboredom added DevDocs [Status] In Progress labels Dec 30, 2016

withinboredom self-assigned this Dec 30, 2016

matticbot added the OSS Citizen label Dec 30, 2016

withinboredom mentioned this pull request Dec 30, 2016

DevDocs/Proptype Index: Generate slug/displayname and capture inline comments #10343

Merged

withinboredom force-pushed the add/proptypes-table branch from f2f4075 to 6666570 Compare January 3, 2017 19:52

withinboredom requested a review from ehg January 3, 2017 20:26

withinboredom force-pushed the add/proptypes-table branch from 6666570 to bb2bc0c Compare January 3, 2017 20:29

lancewillett mentioned this pull request Jan 3, 2017

Add component playground to devdocs #8284

Closed

folletto removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Jan 4, 2017

ehg requested changes Jan 4, 2017

View reviewed changes

ehg reviewed Jan 5, 2017

View reviewed changes

client/blocks/props-viewer/index.jsx Outdated

}

}

PropsViewer.propTypes = {

Copy link

Member

ehg Jan 5, 2017

Choose a reason for hiding this comment

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

These could be put in the class as a static property :]

withinboredom force-pushed the add/proptypes-table branch 2 times, most recently from ec1f3d2 to 85b933a Compare January 9, 2017 17:10

ehg requested changes Jan 10, 2017

View reviewed changes

withinboredom force-pushed the add/proptypes-table branch 4 times, most recently from 6801ffa to fd4e5b7 Compare January 18, 2017 18:59

withinboredom force-pushed the add/proptypes-table branch from fd4e5b7 to 53b275c Compare January 25, 2017 13:03

withinboredom commented Jan 25, 2017

View reviewed changes

ehg previously requested changes Jan 27, 2017

View reviewed changes

withinboredom added 22 commits July 21, 2017 21:25

DevDocs: Generate a table for displaying proptypes

246a8a8

DevDocs: Add css classnames and update table

edc3cba

DevDocs: Add proptype table css

255efbc

DevDocs: Refactor props-viewer

68c9c60

DecDocs: Add tests to props-viewer

19311b0

DevDocs: Add readme for props-viewer component

88f9213

DevDocs: Address review comments

403835d

DevDocs: Use mocked .json file

b32beb6

DevDocs: Move proptypes to the top of the file

30e3a67

DevDocs: Clean up render method some

0c984b5

DevDocs: Remove displayName from PropsViewer component

4186c56

DevDocs: Use new gridicons package

d31b2b9

DevDocs: Remove constructor

58945c5

DevDocs: Remove misc space

d5b0383

DevDocs: Replace some spaces with tabs

494d068

DevDocs: Use lodash find

6d0a522

DevDocs: Fix tests to work with _.find

e2108bb

DevDocs: Break props viewer into smaller components

caf2809

DevDocs: don't mix state and props

b4eda38

DevDocs: Use import statement to require json file

444371d

DevDocs: Use localeCompare

31d7904

DevDocs: Fix an indention

bc67821

withinboredom force-pushed the add/proptypes-table branch from 0557fca to bc67821 Compare July 22, 2017 01:27

withinboredom closed this Oct 3, 2017

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 3, 2017

DanReyLop mentioned this pull request Oct 22, 2017

Improve build performance by removing the "proptypes-index" generation #19064

Merged

alisterscott deleted the add/proptypes-table branch October 24, 2017 04:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DevDocs: Generate a table for displaying proptypes #10342

DevDocs: Generate a table for displaying proptypes #10342

withinboredom commented Dec 30, 2016 •

edited by folletto

Loading

folletto commented Jan 4, 2017

withinboredom commented Jan 4, 2017

ehg commented Jan 4, 2017

ehg left a comment

ehg Jan 4, 2017

withinboredom Jan 5, 2017

ehg Jan 4, 2017

withinboredom commented Jan 5, 2017

ehg Jan 5, 2017

ehg Jan 10, 2017

withinboredom Jan 10, 2017

ehg Jan 10, 2017

withinboredom Jan 10, 2017

ehg Jan 17, 2017

ehg Jan 10, 2017

withinboredom Jan 11, 2017

ehg Jan 17, 2017

withinboredom Jan 18, 2017

ehg Jan 10, 2017

ehg Jan 10, 2017

withinboredom Jan 11, 2017

ehg Jan 10, 2017

withinboredom Jan 11, 2017

aduth Feb 15, 2017

ehg Jan 10, 2017

withinboredom Jan 11, 2017

ehg Jan 17, 2017

withinboredom commented Jan 19, 2017

withinboredom Jan 25, 2017

ehg Jan 27, 2017

withinboredom Jan 30, 2017

ehg Jan 27, 2017

withinboredom commented Oct 3, 2017

DanReyLop commented Oct 22, 2017 •

edited

Loading


		const components = require( '../../../../server/devdocs/proptypes-index.json' );

		const findRealComponent = ( slug ) => {

DevDocs: Generate a table for displaying proptypes #10342

DevDocs: Generate a table for displaying proptypes #10342

Conversation

withinboredom commented Dec 30, 2016 • edited by folletto Loading

folletto commented Jan 4, 2017

withinboredom commented Jan 4, 2017

ehg commented Jan 4, 2017

ehg left a 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

withinboredom commented Jan 5, 2017

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

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

withinboredom commented Jan 19, 2017

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

withinboredom commented Oct 3, 2017

DanReyLop commented Oct 22, 2017 • edited Loading

withinboredom commented Dec 30, 2016 •

edited by folletto

Loading

DanReyLop commented Oct 22, 2017 •

edited

Loading