Skip to content

Commit

Permalink
📖 Assorted issue and PR template fixes (ampproject#34473)
Browse files Browse the repository at this point in the history
  • Loading branch information
rsimha authored and rochapablo committed Aug 30, 2021
1 parent 664383c commit 2e4d47a
Show file tree
Hide file tree
Showing 18 changed files with 70 additions and 79 deletions.
9 changes: 1 addition & 8 deletions .github/ISSUE_TEMPLATE/release-tracker.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,10 @@ body:
This is AMP's release tracker form, meant to be used by on-duty engineers.
- See AMP's [release schedule](https://github.com/ampproject/amphtml/blob/main/docs/release-schedule.md) to learn about how releases work.
- See AMP's [release calendar](https://amp-release-calendar.appspot.com) for information about past releases (e.g. versions, dates, submission times, notes).
- Use this form to track a release through the following stages:
- Initial promotion to Nightly channel
- Promotion to Experimental and Beta opt-in channels
- Promotion to Experimental and Beta 1% traffic channels
- Promotion to Stable channel
- Promotion to LTS channel (if applicable)
- See AMP's pre-release [documentation](https://github.com/ampproject/amphtml/blob/main/docs/release-schedule.md#beta-and-experimental-channels) to learn how to test changes in the Experimental channel.
- See AMP's [pre-release documentation](https://github.com/ampproject/amphtml/blob/main/docs/release-schedule.md#beta-and-experimental-channels) to learn how to test changes in the Experimental channel.
- If you find a bug in this release, file a [bug report](https://github.com/ampproject/amphtml/issues/new?assignees=&labels=Type%3A+Bug&template=bug-report.yml).
- If you believe a bug should be fixed as part of this release, request a [cherry-pick](https://github.com/ampproject/amphtml/blob/main/docs/contributing-code.md#Cherry-picks).
- For updates that may be of interest to the community (e.g. bug fixes, delayed releases), post a comment to this issue.
- The community uses this issue to keep track of releases, so please keep it up to date.
- type: input
id: release_version
attributes:
Expand Down
39 changes: 0 additions & 39 deletions .github/PULL_REQUEST_TEMPLATE.md

This file was deleted.

34 changes: 34 additions & 0 deletions .github/PULL_REQUEST_TEMPLATE/pull_request_template.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<!--
# Instructions:
1. Pick a meaningful title for your pull request.
a. Prefix the title with an emoji. (Copy-paste from the list below.)
b. If helpful, use a short prefix (e.g. `[Project XX] Implement feature YY`).
2. Enter a description that explains why the PR is necessary, and what it does.
a. Mention the GitHub issue being addressed by this pull request.
b. Use keywords to auto-close linked issues during merge. (e.g. `Fixes #11111`, or `Closes #22222`)
3. For substantial changes, first file an Intent-to-Implement (I2I) issue at go.amp.dev/i2i.
# References:
- AMP code contribution docs: go.amp.dev/contribute/code
- First time setup (required for CI checks): go.amp.dev/getting-started
# Emojis for categorizing pull requests (copy-paste emoji into description):
✨ New feature
🐛 Bug fix
🔥 P0 fix
✅ Tests
❄️ Flaky tests
🚀 Performance improvements
🖍 CSS / Styling
♿ Accessibility
🌐 Internationalization
📖 Documentation
🏗 Infrastructure / Tooling / Builds / CI
⏪ Revert
♻️ Refactor
🚮 Deletion
🧪 Experimental code
-->
10 changes: 6 additions & 4 deletions build-system/pr-check/build-targets.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,10 @@ let buildTargets;
/**
* Used to prevent the repeated expansion of globs during PR jobs.
*/
let lintFiles;
let htmlFixtureFiles;
let invalidWhitespaceFiles;
let linkCheckFiles;
let lintFiles;
let presubmitFiles;
let prettifyFiles;

Expand Down Expand Up @@ -176,9 +177,9 @@ const targetMatchers = {
return false;
}
return (
linkCheckFiles.includes(file) ||
file == 'build-system/tasks/check-links.js' ||
file.startsWith('build-system/tasks/markdown-toc/') ||
(path.extname(file) == '.md' && !file.startsWith('examples/'))
file.startsWith('build-system/tasks/markdown-toc/')
);
},
[Targets.E2E_TEST]: (file) => {
Expand Down Expand Up @@ -330,9 +331,10 @@ function determineBuildTargets() {
return buildTargets;
}
buildTargets = new Set();
lintFiles = globby.sync(config.lintGlobs);
htmlFixtureFiles = globby.sync(config.htmlFixtureGlobs);
invalidWhitespaceFiles = globby.sync(config.invalidWhitespaceGlobs);
linkCheckFiles = globby.sync(config.linkCheckGlobs);
lintFiles = globby.sync(config.lintGlobs);
presubmitFiles = globby.sync(config.presubmitGlobs);
prettifyFiles = globby.sync(config.prettifyGlobs);
const filesChanged = gitDiffNameOnlyMain();
Expand Down
5 changes: 3 additions & 2 deletions build-system/test-configs/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -173,10 +173,11 @@ const prettifyGlobs = [
];

/**
* List of markdown files that may be checked by `amp check-links` (using
* markdown-link-check).
* List of markdown and yaml files that may be checked by `amp check-links`
* (using markdown-link-check).
*/
const linkCheckGlobs = [
'.github/ISSUE_TEMPLATE/*.yml',
'**/*.md',
'!**/{examples,node_modules,build,dist,dist.3p,dist.tools}/**',
];
Expand Down
2 changes: 1 addition & 1 deletion docs/amp-module-build.md
Original file line number Diff line number Diff line change
Expand Up @@ -75,4 +75,4 @@ We Also remove the following polyfills:

## Report a bug

[File an issue](https://github.com/ampproject/amphtml/issues/new?assignees=&labels=Type%3A+Bug&template=bug-report.md&title=) if you find a bug in AMP. Provide a detailed description and steps for reproducing the bug; screenshots and working examples that demonstrate the problem are appreciated!
[File an issue](https://github.com/ampproject/amphtml/issues/new?assignees=&labels=Type%3A+Bug&template=bug-report.yml) if you find a bug in AMP. Provide a detailed description and steps for reproducing the bug; screenshots and working examples that demonstrate the problem are appreciated!
8 changes: 4 additions & 4 deletions docs/contributing-code.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ Significant changes (e.g. new components or significant changes to behavior) req

- [ ] _Before you start coding_, [find a guide](#find-a-guide) who you can discuss your change with and who can help guide you through the process.
- [ ] Agree to the [OpenJSF Contributor License Agreement (CLA)](#contributor-license-agreement).
- [ ] File an [Intent-to-implement (I2I)](https://github.com/ampproject/amphtml/issues/new?assignees=&labels=INTENT+TO+IMPLEMENT&template=intent-to-implement--i2i-.md&title=I2I:%20%3Cyour%20change/update%3E) GitHub issue and cc your guide on it. The I2I should include:
- [ ] File an [Intent-to-implement (I2I)](https://github.com/ampproject/amphtml/issues/new?assignees=&labels=INTENT+TO+IMPLEMENT&template=intent-to-implement.yml) GitHub issue and cc your guide on it. The I2I should include:
- A description of the change you plan to implement.
- If you are integrating a third-party service, provide a link to the third-party's site and product.
- Details on any data collection or tracking that your change might require.
Expand All @@ -43,7 +43,7 @@ Significant changes (e.g. new components or significant changes to behavior) req
- Familiarize yourself with our [Design Principles](design-principles.md).
- Your guide can help you determine if your change requires a design doc and whether it should be brought to a [design review](./design-reviews.md).
- [ ] Proceed with the [implementation](#implementation) of your change.
- [ ] For changes that require approval from the Approvers WG, file an [Intent-to-ship (I2S) issue](https://github.com/ampproject/amphtml/issues/new?assignees=&labels=INTENT+TO+SHIP&template=intent-to-ship--i2s-.md&title=I2S:%20%3Cyour%20change/update%3E). Indicate which experiment is gating your change and a rollout plan. Once this issue is approved by 3 members of the Approvers WG the rollout plan described in the I2S may proceed.
- [ ] For changes that require approval from the Approvers WG, file an [Intent-to-ship (I2S) issue](https://github.com/ampproject/amphtml/issues/new?assignees=&labels=INTENT+TO+SHIP&template=intent-to-ship.yml). Indicate which experiment is gating your change and a rollout plan. Once this issue is approved by 3 members of the Approvers WG the rollout plan described in the I2S may proceed.

## Find a guide

Expand Down Expand Up @@ -194,9 +194,9 @@ We have a well-defined process for handling requests for changes to the **experi

The following outlines the process to request a cherry-pick.

- Ensure there is a [GitHub issue](https://github.com/ampproject/amphtml/issues/new/choose) filed for the problem that needs to be resolved _before_ filing the cherry-pick request issue.
- Ensure there is a [bug report](https://github.com/ampproject/amphtml/issues/new?assignees=&labels=Type%3A+Bug&template=bug-report.yml) filed for the problem, and ensure it is resolved _before_ filing the cherry-pick request.
- Escalate the issue to P0 by attaining consensus from one or more members of the [Approvers Working Group (WG)](https://github.com/ampproject/wg-approvers) (if you are a member of this working group, you may not count your voice for consensus purposes)
- File the cherry-pick request issue using the [Cherry-pick request template](https://github.com/ampproject/amphtml/issues/new?title=%F0%9F%8C%B8%20Cherry%20pick%20request%20for%20%3CYOUR_ISSUE_NUMBER%3E%20into%20%3CRELEASE_ISSUE_NUMBER%3E%20%28Pending%29&template=cherry_pick_template.md). Follow the instructions in the template, providing all the requested data. **Make sure you fill out this issue completely or your cherry-pick may not be seen or acted upon.**
- File the cherry-pick request issue using the [Cherry-pick request template](https://github.com/ampproject/amphtml/issues/new?assignees=&labels=Type:+Release,Cherry-pick:+Beta,Cherry-pick:+Experimental,Cherry-pick:+LTS,Cherry-pick:+Stable&template=cherry-pick-request.yml&title=%F0%9F%8C%B8+Cherry-pick+request+for+%23ISSUE+into+%23RELEASE). Follow the instructions in the template, providing all the requested data. **Make sure you fill out this issue completely or your cherry-pick may not be seen or acted upon.**
- Get the necessary approval for your cherry-pick, indicated via comments on the cherry-pick request issue.
- For cherry-picks into **experimental**/**beta**, at least one member of the [Approvers WG](https://github.com/orgs/ampproject/teams/wg-approvers/members) must approve the cherry-pick/rollback.
- For cherry-picks into **stable**/**lts** at least one member from the [Cherry-Pick Approvers group](https://github.com/orgs/ampproject/teams/cherry-pick-approvers/members) must approve the cherry-pick.
Expand Down
8 changes: 4 additions & 4 deletions docs/contributing.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,25 +21,25 @@ How would you like to help?
- [Get started with open source](#get-started-with-open-source)
- [Ongoing participation](#ongoing-participation)

If you have questions about _using_ AMP or are _encountering problems using AMP_ on your site please visit our [support page](SUPPORT.md) for help.
If you have questions about _using_ AMP or are _encountering problems using AMP_ on your site please visit our [support page](./support.md) for help.

## Report a bug

[File an issue](https://github.com/ampproject/amphtml/issues/new?assignees=&labels=Type%3A+Bug&template=bug-report.md&title=) if you find a bug in AMP. Provide a detailed description and steps for reproducing the bug; screenshots and working examples that demonstrate the problem are appreciated!
[File a bug report](https://github.com/ampproject/amphtml/issues/new?assignees=&labels=Type%3A+Bug&template=bug-report.yml) if you find a bug in AMP. Provide a detailed description and steps for reproducing the bug; screenshots and working examples that demonstrate the problem are appreciated!

The community regularly monitoring issues and will try to fix open bugs quickly according to our [prioritization guidelines](./issue-priorities.md).

## Make a suggestion

[File a "feature request" issue](https://github.com/ampproject/amphtml/issues/new?assignees=&labels=Type%3A+Feature+Request&template=feature_request.md&title=) if you have a suggestion for a way to improve AMP or a request for a new AMP feature.
[File a feature request](https://github.com/ampproject/amphtml/issues/new?assignees=&labels=Type%3A+Feature+Request&template=feature-request.yml) if you have a suggestion for a way to improve AMP or a request for a new AMP feature.

If you are suggesting a feature that you are intending to implement, please see the [Contributing code/features](#contribute-code-features) section below for next steps.

## Contribute code/features

We'd love to have your help contributing code and features to AMP!

See the [Contributing code and features](docs/contributing-code.md) document for details on the process you can use to add code/features.
See the [Contributing code and features](./contributing-code.md) document for details on the process you can use to add code/features.

## Get started with open source

Expand Down
2 changes: 1 addition & 1 deletion docs/developing.md
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ To opt your browser into the a pre-release channel, go to [the AMP experiments p

**If you find an issue that appears to only occur in the _Experimental/Beta Channel_ version of AMP**:

- please [file an issue](https://github.com/ampproject/amphtml/issues/new) with a description of the problem
- please [file a bug report](https://github.com/ampproject/amphtml/issues/new?assignees=&labels=Type%3A+Bug&template=bug-report.yml) with a description of the problem
- include a note that the problem is new to the _Experimental/Beta Channel_ build so that it can be properly prioritized
- include a URL to a page that reproduces the problem
- ping the [AMP Slack #release channel](https://amphtml.slack.com/messages/C4NVAR0H3/) ([sign up for Slack](https://bit.ly/amp-slack-signup)) with the issue you filed so we can delay the push of the _Experimental/Beta Channel_ version to production if needed
Expand Down
12 changes: 6 additions & 6 deletions docs/spec/amp-3p-video.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ The generic [`amp-video`](https://go.amp.dev/c/amp-video) component plays videos
Unfortunately, not all videos can be embedded this way. For these specialized cases, AMP provides vendor-optimized components like [`amp-youtube`](https://go.amp.dev/c/amp-youtube), [`amp-ima-video`](https://go.amp.dev/c/amp-ima-video), and [others](./amp-video-interface.md).
Internally these players load an iframe whose page communicates with the outer document to coordinate playback.

**We would prefer to not have additional custom video implementations.** Instead we believe, [`amp-video-iframe`](https://go.amp.dev/c/amp-video-iframe) should be used instead, since it is a generic component that can load any video document to coordinate playback. Instead, new integrations should come in the form of [`amp-video-iframe` configurations](https://go.amp.dev/c/amp-video-iframe#vendors). _If you believe you're unable to integrate with `amp-video-iframe`, please [file an issue](https://github.com/ampproject/amphtml/issues/new/choose) mentioning `@alanorozco` and `@wassgha`._
**We would prefer to not have additional custom video implementations.** Instead we believe, [`amp-video-iframe`](https://go.amp.dev/c/amp-video-iframe) should be used instead, since it is a generic component that can load any video document to coordinate playback. Instead, new integrations should come in the form of [`amp-video-iframe` configurations](https://go.amp.dev/c/amp-video-iframe#vendors). _If you believe you're unable to integrate with `amp-video-iframe`, please [file a bug report](https://github.com/ampproject/amphtml/issues/new?assignees=&labels=Type%3A+Bug&template=bug-report.yml) mentioning `@alanorozco` and `@wassgha`._

## I want to contribute my vendor-specific player

Expand Down Expand Up @@ -52,15 +52,15 @@ For example, these are some of the features that justify specific players:

### Understand the contribution process

You'll need to go through [the standard AMP contribution process](../docs/contributing.md) when creating and submitting your component. For large features, such as a video player, you need to file an [I2I (intent-to-implement) issue](https://github.com/ampproject/amphtml/issues/new?assignees=&labels=INTENT+TO+IMPLEMENT&template=intent-to-implement--i2i-.md&title=I2I:%20%3Cyour%20change/update%3E) that describes the overall workings of your component.
You'll need to go through [the standard AMP contribution process](../contributing.md) when creating and submitting your component. For large features, such as a video player, you need to file an [I2I (intent-to-implement) issue](https://github.com/ampproject/amphtml/issues/new?assignees=&labels=INTENT+TO+IMPLEMENT&template=intent-to-implement.yml) that describes the overall workings of your component.

Your player component is also shipped as an extension, so you should [become familiar with the process of building one.](https://github.com/ampproject/amphtml/blob/main/docs/building-an-amp-extension.md)

### Write your implementation

#### The `VideoManager`

Every player component talks to a single [`VideoManager`](../src/service/video-manager-impl.js) via standard methods (`VideoInterface`) and events (`VideoEvents`) defined in the [AMP video interface](../src/video-interface.js).
Every player component talks to a single [`VideoManager`](../../src/service/video-manager-impl.js) via standard methods (`VideoInterface`) and events (`VideoEvents`) defined in the [AMP video interface](../../src/video-interface.js).

This manager performs standard responsibilities for all videos, regardless of type:

Expand All @@ -72,14 +72,14 @@ This manager performs standard responsibilities for all videos, regardless of ty

#### Interface support

Component classes should implement the [`VideoInterface`](../src/video-interface.js).
Component classes should implement the [`VideoInterface`](../../src/video-interface.js).
You can implement this interface entirely or partially, depending on [the video integration features you'd like to support](./amp-video-interface.md).

At the very least, you should implement `play()` and `pause()`. Likewise, playback
[actions](https://amp.dev/documentation/guides-and-tutorials/learn/amp-actions-and-events/) (like `my-element.play`) will not work when unimplemented.

Read through [the `VideoInterface` code to understand the individual effects of leaving each method unimplemented.](../src/video-interface.js)
Read through [the `VideoInterface` code to understand the individual effects of leaving each method unimplemented.](../../src/video-interface.js)

#### Reference

Existing implementations for [`amp-youtube.js`](../extensions/amp-youtube/0.1/amp-youtube.js) and [`amp-video-iframe.js`](../extensions/amp-video-iframe/0.1/amp-video-iframe.js) are good starter examples for implementation details. They use several standard utilities for creating the video frame and communicating with the `VideoManager`.
Existing implementations for [`amp-youtube.js`](../../extensions/amp-youtube/0.1/amp-youtube.js) and [`amp-video-iframe.js`](../../extensions/amp-video-iframe/0.1/amp-video-iframe.js) are good starter examples for implementation details. They use several standard utilities for creating the video frame and communicating with the `VideoManager`.
Loading

0 comments on commit 2e4d47a

Please sign in to comment.