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.

Sign up for GitHub

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

Jump to bottom

Bento docs: split Example and Import via script sections #36992

Merged
merged 1 commit into from
Nov 19, 2021
Merged

Bento docs: split Example and Import via script sections #36992

merged 1 commit into from
Nov 19, 2021

Conversation

sebastianbenz
Copy link
Contributor

No description provided.

@amp-owners-bot
Copy link

Hey @westonruter, @ediamin! These files were changed:

extensions/amp-wordpress-embed/1.0/README.md

@sebastianbenz sebastianbenz changed the title Bento code snippets Bento docs: split Example and Import via script sections Nov 18, 2021
westonruter
westonruter reviewed Nov 18, 2021
View reviewed changes
extensions/amp-accordion/1.0/README.md Outdated
<script type="nomodule" src="https://cdn.ampproject.org/bento.js"></script>
<script type="module" src="https://cdn.ampproject.org/v0/bento-accordion-1.0.mjs"></script>
<script type="nomodule" src="https://cdn.ampproject.org/v0/bento-accordion-1.0.js"></script>
<link rel="stylesheet" type="text/css" href="https://cdn.ampproject.org/v0/bento-accordion-1.0.css"/>
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for using self-closing tag for link elements?

Suggested change
<link rel="stylesheet" type="text/css" href="https://cdn.ampproject.org/v0/bento-accordion-1.0.css"/>
<link rel="stylesheet" type="text/css" href="https://cdn.ampproject.org/v0/bento-accordion-1.0.css">

It's only relevant for XHTML which is largely from a bygone era.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea where that came from...removed.

westonruter
westonruter reviewed Nov 18, 2021
View reviewed changes
extensions/amp-accordion/1.0/README.md Outdated
<script type="module" src="https://cdn.ampproject.org/bento.mjs"></script>
<script type="nomodule" src="https://cdn.ampproject.org/bento.js"></script>
<script type="module" src="https://cdn.ampproject.org/v0/bento-accordion-1.0.mjs"></script>
<script type="nomodule" src="https://cdn.ampproject.org/v0/bento-accordion-1.0.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

There's an extra space here (and in the other changed files):

Suggested change
<script type="nomodule" src="https://cdn.ampproject.org/v0/bento-accordion-1.0.js"></script>
<script type="nomodule" src="https://cdn.ampproject.org/v0/bento-accordion-1.0.js"></script>

Copy link
Member

Choose a reason for hiding this comment

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

Also, nomodule should be an attribute not a type, right?

Suggested change
<script type="nomodule" src="https://cdn.ampproject.org/v0/bento-accordion-1.0.js"></script>
<script type="text/javascript" nomodule src="https://cdn.ampproject.org/v0/bento-accordion-1.0.js"></script>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the space. nomodule is an attribute.

westonruter
westonruter reviewed Nov 18, 2021
View reviewed changes
extensions/amp-accordion/1.0/README.md Outdated

```html
<script type="module" src="https://cdn.ampproject.org/bento.mjs"></script>
<script type="nomodule" src="https://cdn.ampproject.org/bento.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<script type="nomodule" src="https://cdn.ampproject.org/bento.js"></script>
<script type="text/javascript" nomodule src="https://cdn.ampproject.org/bento.js"></script>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this really needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

("text/javascript")

Copy link
Member

Choose a reason for hiding this comment

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

No idea where that came from! 😉

You're right, remove it. It's not needed.

@sebastianbenz
Copy link
Contributor Author

@westonruter thanks for the review!

@westonruter
Copy link
Member

Also, what about adding crossorigin="anonymous" to all the script and link tags?

@sebastianbenz
Copy link
Contributor Author

Also, what about adding crossorigin="anonymous" to all the script and link tags?

Good call! Completely forgot about this. Added

@sebastianbenz
Copy link
Contributor Author

We need to update all code snippets to follow this pattern.

@sebastianbenz sebastianbenz merged commit 34ebd94 into ampproject:main Nov 19, 2021
@sebastianbenz sebastianbenz deleted the bento-code-snippets branch November 19, 2021 09:29
@westonruter westonruter mentioned this pull request Nov 19, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@westonruter westonruter westonruter approved these changes

@alanorozco alanorozco Awaiting requested review from alanorozco

Assignees
No one assigned
Labels
PR use: In Beta / Experimental PR use: In LTS PR use: In Stable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sebastianbenz @westonruter @ampprojectbot