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

✨ Add amp-wordpress-embed component #24952

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Oct 8, 2019

Previously #18412.

Fixes #18378.

To test the component in the WordPress plugin, use the branch in this PR: ampproject/amp-wp#3465

Todo

  • Add support for link message.
  • Add support for placeholder.
  • Ensure fallback is supported. It should be a blockquote.
  • Add support for overflow so that when the resize is rejected, we can show a button.
  • Verify placeholder works.
  • Figure out the right way to start listening for messages, as listenFor may not be right.
  • Do we need to send a secret?
  • Add assertions for when invalid messages are sent?

@amp-owners-bot
Copy link

amp-owners-bot bot commented Oct 8, 2019

Hey @ampproject/wg-caching, these files were changed:

  • extensions/amp-wordpress-embed/0.1/test/validator-amp-wordpress-embed.html
  • extensions/amp-wordpress-embed/0.1/test/validator-amp-wordpress-embed.out
  • extensions/amp-wordpress-embed/validator-amp-wordpress-embed.protoascii

Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

validation changes look good, just need current year in licenses

@@ -0,0 +1,2 @@
- westonruter
- amedina
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the JSON format for all new OWNERS files. This will become:

// For an explanation of the OWNERS rules and syntax, see:
// https://github.com/ampproject/amp-github-apps/blob/master/owners/OWNERS.example

{
  rules: [
    {
      owners: [
        { name: "westonruter" },
        { name: "amedina" }
      ]
    }
  ]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, /cc @kristoferbaxter in case he thinks we should add ampproject/wg-ui-and-a11y as an owner

Copy link
Contributor

@rcebulko rcebulko left a comment

Choose a reason for hiding this comment

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

Approving OWNERS changes

@westonruter westonruter marked this pull request as ready for review October 9, 2019 21:58
Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

validation changes look good

@mrjoro
Copy link
Member

mrjoro commented Jun 11, 2020

Is this PR ready to go in?

@westonruter westonruter marked this pull request as draft June 11, 2020 18:01
@westonruter
Copy link
Member Author

No, still on our radar to finish. Most likely to be picked up toward end of Q3.

@CLAassistant
Copy link

CLAassistant commented Jan 26, 2021

CLA assistant check)
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement) before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ westonruter
❌ amedina
You have signed the CLA already but the status is still pending? Let us recheck) it.

@Gregable
Copy link
Member

Gregable commented Feb 1, 2021

@westonruter is this PR still active?

@westonruter
Copy link
Member Author

No. Now that Bento is underway, it'll need to be rewritten to make use of the new APIs anyway.

@westonruter westonruter closed this Feb 2, 2021
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.

Intent to Implement: <amp-wordpress-embed> Embedding WordPress Posts
8 participants