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

Nested amp-iframe in amp-sidebar #6452

Closed
johandeklerk opened this issue Dec 2, 2016 · 13 comments
Closed

Nested amp-iframe in amp-sidebar #6452

johandeklerk opened this issue Dec 2, 2016 · 13 comments

Comments

@johandeklerk
Copy link

johandeklerk commented Dec 2, 2016

We have a use case where we use amp-sidebar to display non-amp content in a amp-iframe. According to the validator this is not allowed.

<amp-sidebar id='search' layout="nodisplay" side="right">

<amp-iframe src="http://someurl"
            height="120"
            width="30"
            layout="responsive"
            frameborder="0"
            sandbox="allow-scripts allow-top-navigation">
    <amp-img src="/img/loading.png"
             layout="fixed-height"
             height="700"
             width="auto"
             placeholder></amp-img>
</amp-iframe>

</amp-sidebar>

Is this something that will be supported in future? Or is there a way around this?

@johandeklerk johandeklerk changed the title amp-iframe Nested amp-iframe in amp-sidebar Dec 2, 2016
@jpettitt
Copy link
Contributor

jpettitt commented Dec 2, 2016

Can we please just allow any valid amp markup in amp-sidebar? including amp-ad, amp-iframe etc?

@ericlindley-g
Copy link
Contributor

We initially restricted this in order to ensure good UX (e.g. can't use the sidebar as a hack to create a blocking ad or newsletter signup on first page load).

What are some use-cases you'd like to support, @johandeklerk and @jpettitt ? Whitelisting components might be the right approach, but there could be a more focused, lower-risk solution.

@ericlindley-g
Copy link
Contributor

/cc @camelburrito

@johandeklerk
Copy link
Author

Thanks for the feedback. So our UI design has a search form in a sidebar which is not AMP compliant. So to keep the main page 100% valid AMP we thought we would just load the iframe content inside the sidebar. But the validator doesn't like that so had to revert to actually putting the search form on a separate page. Not a nice UX.

@camelburrito
Copy link
Contributor

Have you considered using a light box for this ?

@ericlindley-g
Copy link
Contributor

@johandeklerk Thanks for explaining — and just to make sure I understand, does the search form have to be done through an iframe, or could it be done using amp-form?

@jpettitt
Copy link
Contributor

jpettitt commented Dec 5, 2016

First off why is a newsletter signup not a valid use - this is a user action initiated feature, it won't roadblock a page. I'd love to be able to have a signup form behind a button. Other use cases include a list of articles that may include a native content ad (eg nativo). Given that we could use a lightbox I'm not clear why one form of page takeover is ok and another isn't. It seems to me that trying to legislate content by banning markup is a losing battle. (see prior discussion around * selector).

It's as much a philosophy thing as anything else - where something is forbidden is thould be explained clearly why in the doc and if you can't come up with a reason that's backed by research it should be allowed.

@johandeklerk
Copy link
Author

@ericlindley-g No we need some custom js on the form unfortunately so it has to be through an iframe. I did try my best to make it work with amp-form but the design just doesn't lend itself well to using amp-form. We are looking into using a lightbox to achieve the same effect but it feels like a bit of a hack.

@Gregable
Copy link
Member

Gregable commented Feb 7, 2017

@adelinamart I can implement, but I'm not sure if a decision has been reached. I'll assign @ericlindley-g for now.

@ericlindley-g
Copy link
Contributor

@johandeklerk and @jpettitt , you make good points:

Considering that this feature is triggered by user interaction ( @camelburrito , can you confirm that this is true? I don't think applying "open" to the sidebar means that it opens on load, as I have not been able to repro that behavior), I think there's low risk in allowing amp-form and amp-iframe inside of it. WDYT, @camelburrito?

/cc @rudygalfi and @cramforce for visibility, in case we're missing some risk.

@cramforce
Copy link
Member

There is no way to open it initially, right?

I actually kind of always argued to not make any special rules for the sidebar.

@ericlindley-g
Copy link
Contributor

I believe this is implemented — please reopen or create a new issue if there are issues that aren't yet addressed.

@Gregable
Copy link
Member

Fixed in #11070

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

No branches or pull requests

8 participants