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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃摉 Fix amp-script doc #29582

Merged
merged 4 commits into from Jul 30, 2020
Merged

Conversation

sebastianbenz
Copy link
Contributor

  • normalize headings
  • fix broken raw tag
  • mention toolbox csp lib instead of suggesting implementing on your own
  • remove experimental flag

* normalize headings
* fix broken raw tag
* mention toolbox csp lib instead of suggesting implementing on your own
* remove experimental flag
Copy link
Contributor

@morsssss morsssss 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 doing this!

I do have one suggestion....

@@ -32,7 +31,7 @@ An `amp-script` element can load JavaScript in two ways:
- remotely, from a URL
- locally, from a `<script>` element on the page

#### Load JavaScript from a remote URL
### Load JavaScript from a remote URL
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing these! This was also on my to-do list. Now I don't have to :)


```js
function generateCSPHash(script) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea to suggest that people do this by importing the toolbox. Especially as then they'll be up-to-date if somehow we ever change the way this is calculated. I did also change the docs to mention the toolbox earlier.

That said, I think people should also know how to do this themselves - and providing the sample code is more explicit than the instructions I gave.

Why don't we include both methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Developers should use the toolbox library instead of implementing it on their own. This gives us an easier upgrade path if we'd decide to change the implementation in the future. If they want to see the implementation they can look at the source code of the NPM module.

Copy link
Contributor

@morsssss morsssss left a comment

Choose a reason for hiding this comment

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

thanks!

one miniscule, tiny nit remains...

@@ -205,6 +206,8 @@ function generateCSPHash(script) {
}
```

There also a node module available which does it for you: [@ampproject/toolbox-script-csp](https://www.npmjs.com/package/@ampproject/toolbox-script-csp).
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change "There also" to "There is also"?

Copy link
Contributor

@morsssss morsssss 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 fixing what I somehow managed to break...

@sebastianbenz sebastianbenz merged commit 0d15009 into ampproject:master Jul 30, 2020
@sebastianbenz sebastianbenz deleted the fix-amp-script branch July 30, 2020 19:15
ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020
* 馃摉 Fix amp-script doc

* normalize headings
* fix broken raw tag
* mention toolbox csp lib instead of suggesting implementing on your own
* remove experimental flag

* fix more indentation

* re-add node snippet

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

Successfully merging this pull request may close these issues.

None yet

3 participants