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

📖 updated extension generator markdown template #25459

Merged
merged 10 commits into from Nov 19, 2019
Merged

📖 updated extension generator markdown template #25459

merged 10 commits into from Nov 19, 2019

Conversation

CrystalOnScript
Copy link
Contributor

This PR updates the template used to create reference documentation for new amp components.

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

LGTM overall. I'll let other reviewers approve the contents.

Can you sanity check this to make sure the extension generator doesn't conflict with prettier?

# Generate an extension
gulp make-extension --name amp-crystal

# Make sure there are no formatting errors
gulp prettify --files "extensions/amp-crystal/**.md"

If all goes well, the second step should not yield any formatting errors.

If there are errors, you'll have to either adjust extension-doc.template.md, or modify this code:

const getMarkdownDocFile = async name =>
(await fs.readFile(`${__dirname}/extension-doc.template.md`))
.toString('utf-8')
.replace(/\\\$category/g, '$category')
.replace(/\\?\${name}/g, name)
.replace(/\\?\${year}/g, year);

Copy link
Contributor

@sebastianbenz sebastianbenz left a comment

Choose a reason for hiding this comment

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

Left a few comments. I think it'd be good to simplify the structure:

  • only once include an executable example template. If it makes sense, mention it in the section description instead.
  • explicitly mark all optional sections

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Thanks for all the due diligence!

@@ -79,6 +151,114 @@ FILL THIS IN. Does this extension allow for properties to configure?
</tr>
</table>

### optional-attribute-name (optional)

Here, I write what optional-attribute-name will do to amp-component-name.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: wrap the attribute names in `


### action-name

Description of action. Use cases of action-name. Include all the nuances, such as: amp-component-name needs to be identified with an `id` to work.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: wrap the action names in `


Description of attribute. Use cases for this attribute.

- attribute-value-option-one (default): attribute-option-one-value does this to amp-component-name.
Copy link
Contributor

Choose a reason for hiding this comment

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

should the value be wrappen in `?

Copy link
Contributor

@sebastianbenz sebastianbenz left a comment

Choose a reason for hiding this comment

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

Few nits left. Otherwise LGTM

@@ -73,6 +71,11 @@ limitations under the License.

[/example][/filter]
Copy link
Contributor

Choose a reason for hiding this comment

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

add line break before filter

@CrystalOnScript CrystalOnScript merged commit 4616614 into ampproject:master Nov 19, 2019
@CrystalOnScript CrystalOnScript deleted the update-template branch November 19, 2019 22:24
micajuine-ho pushed a commit to micajuine-ho/amphtml that referenced this pull request Dec 27, 2019
* updated template

* prettiefied woot

* fixed prettify generation

* updated template

* prettyfied template

* moved problem paragraph under frontmatter

* spacing issue

* resolved comments

* removed table option and excess example blocks

* fixed nits
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants