Adding GoogleMaps and Youtube embed components #52909
Conversation
| const Page = () => { | ||
| return ( | ||
| <div class="container"> | ||
| <h1>Youtube Embed</h1> |
There was a problem hiding this comment.
| <h1>Youtube Embed</h1> | |
| <h1>Google Maps Embed</h1> |
| const Page = () => { | ||
| return ( | ||
| <div class="container"> | ||
| <h1>Youtube Embed</h1> |
There was a problem hiding this comment.
| <h1>Youtube Embed</h1> | |
| <h1>Google Maps Embed</h1> |
| import { YoutubeEmbed } from '@next/third-parties/google' | ||
|
|
||
| export default function Page() { | ||
| return <YoutubeEmbed videoid="ogfYd705cRs" height={400} /> |
There was a problem hiding this comment.
@janicklas-ralph Are we planning on changing this in this PR or a follow-up?
| <ThirdPartyScriptEmbed | ||
| height={args.height || null} | ||
| width={args.width || null} | ||
| content={`<iframe loading="lazy" src="https://www.google.com/maps/embed/v1/${args.mapMode}?key=${args.apiKey}&${args.parameters}" width=${args.width} height=${args.height} style=${args.style} loading=${args.loading} allowfullscreen=${args.allowfullscreen} referrerpolicy="no-referrer-when-downgrade"></iframe>`} |
There was a problem hiding this comment.
Looks like there are duplicate loading attributes defined in the HTML here - one is "lazy" and one comes from "args.loading". I imagine we should combine the logic into one attribute so we fallback to the "lazy" if there isn't an override value?
There was a problem hiding this comment.
@janicklas-ralph Yeah this shouldn't be an issue anymore once you've upgraded to 1.0.19. I've separated out all the attributes instead of having everything in one single content property which should make adding overrides easier.
There was a problem hiding this comment.
LGTM. We'll have to modify some of this logic a bit to support the new Third Party Capital schema in v1.0.19 but like we discussed, we can do that in a follow-up PR :)
| <ThirdPartyScriptEmbed | ||
| height={args.height || null} | ||
| width={args.width || null} | ||
| content={`<iframe loading="lazy" src="https://www.google.com/maps/embed/v1/${args.mapMode}?key=${args.apiKey}&${args.parameters}" width=${args.width} height=${args.height} style=${args.style} loading=${args.loading} allowfullscreen=${args.allowfullscreen} referrerpolicy="no-referrer-when-downgrade"></iframe>`} |
There was a problem hiding this comment.
@janicklas-ralph Yeah this shouldn't be an issue anymore once you've upgraded to 1.0.19. I've separated out all the attributes instead of having everything in one single content property which should make adding overrides easier.
| import { YoutubeEmbed } from '@next/third-parties/google' | ||
|
|
||
| export default function Page() { | ||
| return <YoutubeEmbed videoid="ogfYd705cRs" height={400} /> |
There was a problem hiding this comment.
It looks like we downgraded back to 1.0.17 in this PR. I think the plan is to do this in a follow-up PR once upgrading to the latest version.
There was a problem hiding this comment.
This file seems not being used and can be deleted?
test/e2e/third-parties/index.test.js
Outdated
| @@ -0,0 +1,32 @@ | |||
| import { createNextDescribe } from 'e2e-utils' | |||
There was a problem hiding this comment.
can we change this test file to typescript like others to avoid potential mistakes in test files
| /** | ||
| * @type {import('next').NextConfig} | ||
| */ | ||
| const nextConfig = {} |
There was a problem hiding this comment.
we can also skip the next config file as it's using the default config
Adding GoogleMapsEmbed and YoutubeEmber components into
@next/third-partiesAdding tests and README
Each component is tagged with
data-ntpcattributecc: @kara @housseindjirdeh @huozhi @gnoff