Amphtml: Enum for AMP events

[choumx]

First Timers Only

We know figuring out the process for contributing to an open source project can be intimidating, so we created this issue as a way for you to learn the ropes. (If you feel comfortable contributing to open source projects, please leave this issue for someone else.)

What you will need to know

• Some familiarity with JavaScript.

Background

The DOM has a rich event model, e.g. for handling user input. AMP also uses custom events for coordinating behavior across components and modules in the code base. These AMP events follow a naming convention being prefixed with amp:, e.g. amp:built.

Motivation

Most usages of amp: events today use the string literal directly, for example on this line.

this.element.dispatchCustomEvent('amp:built');

This is nice and simple, but it results in duplication of the amp:built string and has the risk of typos. A better way to do this is to define these events in an enum, like what video-interface#VideoEvents does.

The bug

Let's create a new file, src/amp-events.js, that contains an enum defining the commonly used events for AMP components. This will reduce duplication, eliminate the risk of typos, and improve readability of our code base.

Step by step

• [x] Claim this issue by adding a comment below. Please only claim this bug if you plan on starting work in the next day or so.
• [ ] If you aren't too familiar with Git/GitHub, see the Getting Started End-to-End Guide for an intro to Git & GitHub, and how to get a copy of the code. You can also refer to the Quick Start Guide for the necessary setup steps with less explanation than the End-to-End guide.
• [ ] Follow the instructions for building AMP.
• [ ] [Create a Git branch](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#create-a-git-branch) for making your changes.
• [ ] Search the code base for AMP events (try searching for 'amp:).
• [ ] Create the src/amp-events.js file with an object literal enumerating the events you've found in your search.
• [ ] Import your new file into files containing the existing string literal event names and replace them with a reference to your enum.
• [ ] [Commit your changes](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#edit-files-and-commit-them) frequently.
• [ ] [Push your changes to GitHub](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#push-your-changes-to-your-github-fork).
• [ ] [Create a Pull Request](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#send-a-pull-request-ie-request-a-code-review). Mention Fixes #9738 in the description and mention me (@choumx) to get a code review.
• [ ] [Respond to your reviewer's comments](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#respond-to-pull-request-comments) (if any).

Once approved, your changes will be merged. ⚡⚡⚡Congrats on making your first contribution to the AMP Project!⚡⚡⚡ You'll be able to see it live across the web soon!

Thanks, and we hope to see more contributions from you soon.

Questions?

If you have questions ask in this issue or on your Pull Request (if you've created one) or see the How to get help section of the Getting Started guide. Mention me (@choumx) so I can help!

[abc516]

Hi @choumx , I would like to claim this issue.

[abc516]

Hi @choumx . I made the changes as specified in the Pull Request. However, the changes I've made are making the Travis-CI build fail.

It seems adding the src/amp-events.js file as a dependency to various files in the 3p/ and extensions/ folders is violating dependency injection rules. Is it ok if I whitelist these files in build-system/dep-check-config.js?

[choumx]

@abc516 That should be fine since it's just an enum. Thanks!

[mrjoro]

Hi @abc516; I sent you an invitation to join ampproject on GitHub; once you accept that I think we'll be able to assign this issue to you. Thanks!

[abc516]

OK Joey, I just joined.

On Mon, Jun 19, 2017 at 12:44 PM, Joey Rozier [e-mail]
wrote:

Hi @abc516 https://github.com/abc516; I sent you an invitation to join
ampproject on GitHub; once you accept that I think we'll be able to assign
this issue to you. Thanks!

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/ampproject/amphtml/issues/9738#issuecomment-309497989,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AH99A4wquoiyuNlYYx5Nhja7SwOZ_DoFks5sFqVxgaJpZM4NxkSF
.

[mrjoro]

Thanks @abc516!

[abc516]

You are welcome, @mrjoro !