Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Documentation: Try a new custom documentation tool to rule them all #1786

Merged
merged 56 commits into from
Jul 10, 2017

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Jul 7, 2017

Requirements we're trying to achieve here:

  • A unique documentation tool for all our docs
  • Contains the creating blocks guide
  • Code examples are written in ES5 and ESnext.
  • Has to live in the repository
  • Integrate Well with the WordPress.org docs
  • Will contain the pattern library
    • Loads and runs React Components
    • Can change components' props live
  • Will be searchable
  • Can support advanced documentation (for example Redux's selectors discovery)

This PR adds a Docs tool called docutron you can launch like that:

npm run docs-start

The docs folder has an index.js as an Entry point where you can add "stories" to your documentation website:

import { addStory } from 'glutenberg';
import myReadme from '../readme.md';
addStory( {
  name: 'intro',
  title: 'Introduction',
  markdown: myReadme,
} );

For more flexibility, a story can provide a React Component instead of providing a markdown string.

screen shot 2017-07-07 at 17 33 21

@youknowriad youknowriad added the [Type] Developer Documentation Documentation for developers label Jul 7, 2017
@youknowriad youknowriad self-assigned this Jul 7, 2017
@youknowriad youknowriad requested a review from aduth July 7, 2017 15:34
@nylen
Copy link
Member

nylen commented Jul 7, 2017

Integrate Well with the WordPress.org docs

This is not something to solve today. However, what is the current plan for uploading this documentation to developer.wordpress.org? How do you see this integration going?

When we do that, let's clean up code samples on that site. Right now the font size is so big that they are almost impossible to usefully read.

@youknowriad youknowriad force-pushed the try/new-docs-tool branch 2 times, most recently from 86ac163 to 57bc239 Compare July 7, 2017 15:57
@youknowriad
Copy link
Contributor Author

This is not something to solve today. However, what is the current plan for uploading this documentation to developer.wordpress.org? How do you see this integration going?

Today, we're trying to just have a similar styling, and maybe we just embed an iframe in the WordPress docs since it's have the same styling.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling f9e8caa on try/new-docs-tool into ** on master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 01d229c on try/new-docs-tool into ** on master**.

@youknowriad youknowriad changed the title [WIP] Documentation: Try a new custom documentation tool to rule them all Documentation: Try a new custom documentation tool to rule them all Jul 8, 2017
@youknowriad
Copy link
Contributor Author

The documentation tool is almost ready. I need some eyes here cc @aduth
I wonder if we should merge this without completing the blocks guide.
Also, once merged, we'll have to PR and remove Storybook and update the deployment hook to deploy this instead.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling debed01 on try/new-docs-tool into ** on master**.


return (
<div>
<div>
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the nested div ?

}

componentDidUpdate() {
Prism.highlightAll();
Copy link
Member

Choose a reason for hiding this comment

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

Can we be more targeted about this? Either passing the individual node or the code content?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going to leave this for now, because we technically don't know if there's a code content in the current tabs, these tabs can contain any markup.


class Page extends Component {
componentDidMount() {
Prism.highlightAll();
Copy link
Member

Choose a reason for hiding this comment

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

Seems odd to call code highlighting here, and not in the tabs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is necessary when you switch pages that contain code blocks without tabs

<div className="navigation">
{ !! previousStory && (
<p className="nav-older">
<Link to={ previousStory.path }>{ '←' } { previousStory.title }</Link>
Copy link
Member

Choose a reason for hiding this comment

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

Does need to be wrapped as an expression?

Copy link
Member

Choose a reason for hiding this comment

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

The core docs include some extra hints here, like rel="previous" and rel="next".

https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types

Copy link
Contributor Author

@youknowriad youknowriad Jul 8, 2017

Choose a reason for hiding this comment

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

The wrapping makes it more visible in my IDE. The same sign is used to display "tabs". But I can live without it if necessary

@@ -0,0 +1,21 @@
# See https://help.github.com/ignore-files/ for more about ignoring files.
Copy link
Member

Choose a reason for hiding this comment

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

Should docs-tool be a top-level folder, or could we move it within docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should move it outside the repository later, glutenberg maybe :)


// Adding the markdown loader and exclude if from the file loader
webpackConfig.module.rules.forEach( rule => {
if ( rule.loader === require.resolve( 'file-loader' ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Should loaders be defined in docs-tool/package.json if we're treating it as a separate project, or are these guaranteed to exist by create-react-app?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think CRA guarantee this since it's using it.

constructor() {
super( ...arguments );
this.state = {
activeTab: 0,
Copy link
Member

Choose a reason for hiding this comment

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

A future / present feature which would be nice to support is remembering preference across each page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem here is that the tabs can be anything not always (es5, es6)

return (
<div id="secondary" className="widget-area">
<div className="secondary-content">
<aside id="handbook_pages-3" className="widget widget_wporg_handbook_pages">
Copy link
Member

Choose a reason for hiding this comment

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

Think we can drop the id.

@@ -0,0 +1,13 @@
import React from 'react';
import ReactDOM from 'react-dom';
import 'prismjs';
Copy link
Member

Choose a reason for hiding this comment

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

Can we see if we can get rid of this? Not sure why we need both this and Prism.highlightAll(). Shouldn't highlightAll only be occurring when the code has been rendered anyways?

@@ -0,0 +1,47 @@
import { addStory } from 'glutenberg';
Copy link
Member

Choose a reason for hiding this comment

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

Will we have more than one file in this folder? Should we start as docs/stories.js to avoid prematurely nesting directories?

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 830c99a on try/new-docs-tool into ** on master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 830c99a on try/new-docs-tool into ** on master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 2455a663199eaa446c71381609ffeed40a9aaf7d on try/new-docs-tool into ** on master**.

@youknowriad youknowriad force-pushed the try/new-docs-tool branch 2 times, most recently from 944bfed to 1295aa9 Compare July 10, 2017 08:51
@coveralls
Copy link

Coverage Status

Coverage remained the same at 17.816% when pulling 1295aa972412f099381bce616920c867a77529c2 on try/new-docs-tool into d157b5e on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 17.816% when pulling 1295aa972412f099381bce616920c867a77529c2 on try/new-docs-tool into d157b5e on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 17.816% when pulling 0748c49843d05fff3e7cd1437de974388e0b018a on try/new-docs-tool into d157b5e on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 17.816% when pulling 2adf01f5153b18887648adb71c6e49ca3a5ea46e on try/new-docs-tool into d157b5e on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 17.816% when pulling 4a4a749569b47c1d69be8e3cf17d194e24f6a3cb on try/new-docs-tool into d157b5e on master.

const path = require( 'path' );

module.exports = function( webpackConfig, usersCwd ) {
const docsFolder = 'docs';
Copy link
Member

Choose a reason for hiding this comment

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

Should this be hard-coded as docs, or should we support specifying this from the command-line?

@@ -0,0 +1,14 @@
#!/usr/bin/env node
const path = require( 'path' );
Copy link
Member

Choose a reason for hiding this comment

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

Should we add DocBlocks for dependencies ("External dependencies", etc)? For consistency with other "modules".

import React, { Component } from 'react';
import { Link } from 'react-router-dom';

import { getNextStory, getPreviousStory } from 'docutron';
Copy link
Member

Choose a reason for hiding this comment

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

It seems a bit odd for this module to depend from itself. Should we instead import by traversing to the relative path?

import markdown from '../markdown';
import Tabs from './Tabs';

function MarkdownContent( { content } ) {
Copy link
Member

Choose a reason for hiding this comment

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

We might consider extracting this component to a separate file.

@@ -0,0 +1,12 @@
import React from 'react'; // eslint-disable-line no-unused-vars
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't need eslint-disable-line no-unused-vars anymore.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

This appears to be in a good state for an initial pass at documentation. Let's get this in and continue iterating.

@karmatosed
Copy link
Member

Just checking on this, we have 2 issues for the pattern library here: https://github.com/WordPress/gutenberg/labels/Pattern%20Library. Is the idea that this works as that? I am trying to get context and don't want to duplicate if this is already happening.

@aduth
Copy link
Member

aduth commented Jul 18, 2017

@karmatosed I think the work here relates to the pattern library, but doesn't necessary supersede it. Rather, this merely sets a base for consolidating documentation, which we can build out to include a components library similar to Calypso DevDocs, homegrown in the same fashion. The problem with Storybook is that it worked particularly well at this one role of component demonstrations, but wouldn't allow us to scale up to include more forms of documentation, or style in a fashion that would be easy to integrate with the WordPress developer site.

@karmatosed
Copy link
Member

@aduth thank you for explaining a little. I am really interested in anything that makes things easier :) How can we unite the approaches? I really would like to do that. Maybe we can take it to those tickets though, if that is more suitable?

@aduth
Copy link
Member

aduth commented Jul 18, 2017

Yes, let's move discussion into issues. I've left another comment at #1706 (comment)

Tug pushed a commit that referenced this pull request Feb 28, 2020
…_bridge_and_aztec

Hardcode Android RN version in bridge and aztec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants