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

AMP Support For Polls & Surveys #41

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

jgcaruso
Copy link
Contributor

@jgcaruso jgcaruso commented Sep 3, 2020

This PR replaces poll js embed rendering with an amp-iframe when the page is being served via AMP.

Changes proposed in this Pull Request:

  • convert crowdsignal poll shortcode and oembeds to an amp-iframe instead of a poll js embed (markup taken from what WordPress renders by default, with some changes to support iframe resizing)

Testing instructions:

The Official AMP plugin needs to be installed on your site (or test on WPCOM) OR AMP for WP – Accelerated Mobile Pages plugin

  1. Add a Poll embed to a post (paste a poll url in an /embed block, or just in a new line on the page)
  2. Add a Poll shortcode to a post [crowdsignal poll=10603231]
  3. View the public post using the /amp endpoint and the poll should render correctly (as an iframe)
  4. View the public post NOT using the /amp endpoint and the poll should render correctly (as a js embed)

Known issues

  • if the amp-iframe is too close to the top of the page, it isn't allowed to render (an AMP rule, you will see an error stating this in the console if it happens)
  • if there are a lot of amp-iframes on a page they may not all resize 🤷

Proposed changelog entry for your changes:

@jgcaruso jgcaruso self-assigned this Sep 9, 2020
@jgcaruso jgcaruso marked this pull request as ready for review September 9, 2020 18:51
@jgcaruso jgcaruso changed the title AMP Support AMP Support For Polls Sep 9, 2020
@@ -178,6 +178,10 @@ function polldaddy_shortcode( $atts ) {
}
} elseif ( intval( $poll ) > 0 ) { //poll embed

if ( cs_is_amp_page() ) {
return sprintf( '<amp-iframe src="https://poll.fm/%d/embed" frameborder="0" height="400" layout="fixed-height" width="auto" sandbox="allow-scripts allow-same-origin" style="height: 400px; --loader-delay-offset:406ms !important;" i-amphtml-layout="fixed-height"></amp-iframe>', $poll );
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we had this conversation about surveys, but any way we can have the amp-iframe autoresize? It gets cut off:

Screen Shot 2020-09-14 at 9 35 45 AM

Copy link
Contributor Author

@jgcaruso jgcaruso Sep 14, 2020

Choose a reason for hiding this comment

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

Looks like there is support for resizing... involves changes to the amp-iframe and the content it is loading.
https://amp.dev/documentation/examples/components/amp-iframe/#resizable-iframe

I'll give this a try and see how it works.

Copy link
Contributor Author

@jgcaruso jgcaruso Sep 14, 2020

Choose a reason for hiding this comment

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

@roundhill thanks for asking about this again. I was able to add support to the poll iframe.

Might be able to do the same with our surveys. Just want to get you to test it on polls to make sure I'm on the right track 😄

See testing instructions for the Crowdsignal side changes that are required.

@jgcaruso jgcaruso changed the title AMP Support For Polls [WIP] AMP Support For Polls (& maybe surveys) Sep 18, 2020
@CGastrell
Copy link
Contributor

Ok, so, embedding a survey got me to reproduce the error you mentioned on slack? (p1600463017056500-slack-C9W0QF6KA)

First thing to notice is that the iframe is not resizing to show all the content:
image

Then noticed I can't submit the survey, but saw this error on the console:
image

This time I couldn't hard-hack a change URL on the form action attribute, but I guess we can pick it up from here to see how to fix the "form's frame is sandboxed and the 'allow-forms' permission is not set."

@jgcaruso
Copy link
Contributor Author

Ok, so, embedding a survey got me to reproduce the error you mentioned on slack? (p1600463017056500-slack-C9W0QF6KA)

First thing to notice is that the iframe is not resizing to show all the content:

Then noticed I can't submit the survey, but saw this error on the console:

This time I couldn't hard-hack a change URL on the form action attribute, but I guess we can pick it up from here to see how to fix the "form's frame is sandboxed and the 'allow-forms' permission is not set."

Ah thanks for finding that! I was looking in the console but didn't notice anything, but maybe because its in an iframe theres a separate console (I feel like FF and Chrome expose those in different ways).

I can take it from there. sandbox and allow-forms is at least something I can research more to see what needs to be done to support them.

@jgcaruso
Copy link
Contributor Author

Form submission is now working, but the survey resize doesn't seem 100% reliable.

Works well on a standard survey, but not working for the Start page... even though the js is executing (confirmed by putting in some console.log statements) but the amp-iframe seems to be ignoring the resize requests.

@jgcaruso jgcaruso changed the title [WIP] AMP Support For Polls (& maybe surveys) AMP Support For Polls (& maybe surveys) Oct 8, 2020
@jgcaruso jgcaruso changed the title AMP Support For Polls (& maybe surveys) AMP Support For Polls & Surveys Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants