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-vimeo] Added special case handler for Vimeo's new h param #37886

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

cahirodoherty-learningpool

Fixes #37618

Vimeo have added a h param to new videos, which will break in iFrame handling due to encodeURIComponent

@amp-owners-bot amp-owners-bot bot requested a review from nainar March 17, 2022 09:36
@CLAassistant
Copy link

CLAassistant commented Mar 17, 2022

CLA assistant check
All committers have signed the CLA.

);
let identifier = encodeURIComponent(videoid);

const paramParts = videoid?.split('?h=') || [];
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should make this handle more url params other then just h. I think it'd be possible to generalize this to handle any url param.

Copy link
Member

Choose a reason for hiding this comment

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

Along these lines, I'm not certain that this should be part of the videoid param. The partial parsing scheme here is a bit awkward.

@cahirodoherty-learningpool do you have an example of where this value comes from? Is it pasted from a URL, or does it come from an "embed code" modal in the Vimeo UI? This would be useful to know so that we can map the interface properly.

Choose a reason for hiding this comment

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

Hi @alanorozco, this is indeed a url from an embed code (provided by a client).

Copy link
Member

Choose a reason for hiding this comment

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

In what context is the URL acquired? Do you have an example or two?

Choose a reason for hiding this comment

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

@dethstrobe I agree, the single parameter splitting here is a little strange, especially as it is referred to as 'videoid' incoming.

@alanorozco The use case was a client dropping an embed url of a private Vimeo video into our app, which parses it for the url. Upon investigation, it seems the 'share link' url to the video will work, so perhaps our app can swap out one for the other instead.

I still think url param handling may be necessary for the amp-vimeo component in some capacity, as Vimeo allows the ability to pass some things like '?quality=720p'. In its current form, this PR is not currently fit for that purpose. Happy to take direction on this issue.

@cahirodoherty-learningpool
Copy link
Author

Hi guys, any further thoughts on this issue?

Copy link

stale bot commented Mar 17, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Inactive for one year or more label Mar 17, 2024
@cahirodoherty-learningpool
Copy link
Author

bump to keep PR from auto-close

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Inactive for one year or more
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[amp-vimeo] New Vimeo 'h' parameter gets encoded
4 participants