Skip to content
This repository has been archived by the owner on Aug 13, 2023. It is now read-only.

Amp validation allowfullscreen attribute #2281

Merged
merged 8 commits into from
Oct 2, 2019
Merged

Amp validation allowfullscreen attribute #2281

merged 8 commits into from
Oct 2, 2019

Conversation

ibMadbouly
Copy link
Contributor

@ibMadbouly ibMadbouly commented Oct 1, 2019 •
edited
Loading

partially Resolves #4044 on simorgh

Code changes:

  • I have assigned myself to this PR and the corresponding issues
  • Automated jest tests added (for new features) or updated (for existing features)
  • This PR requires manual testing

@ibMadbouly ibMadbouly added ws-media The World Service media stream ws-media- LiveRadioV1 labels Oct 1, 2019
@ibMadbouly ibMadbouly self-assigned this Oct 1, 2019
@ibMadbouly ibMadbouly marked this pull request as ready for review October 1, 2019 13:50
DenisHdz
DenisHdz reviewed Oct 1, 2019
View reviewed changes
Copy link
Contributor

@DenisHdz DenisHdz left a comment

Choose a reason for hiding this comment

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

Have you tried to use allowFullScreen in camelCase format?

@ibMadbouly
Copy link
Contributor Author

ibMadbouly commented Oct 1, 2019 •
edited
Loading

@DenisHdz as atrribute value ?

@DenisHdz
Copy link
Contributor

DenisHdz commented Oct 1, 2019

@DenisHdz as atrribute value ?

Yep, just give it a go. Otherwise, looks good to do allowfullscreen="allowfullscreen"

<amp-iframe
     sandbox="allow-scripts allow-same-origin"
     layout="fill"
     frameborder="0"
     src={src}
     allowFullScreen
>

@ibMadbouly
Copy link
Contributor Author

allowFullScreen

tried it and it does not work

pjlee11
pjlee11 approved these changes Oct 1, 2019
View reviewed changes
Copy link
Contributor

@pjlee11 pjlee11 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Tried this locally by updating the Simorgh node_modules to have the dist output with these changes and seeing the allowfullscreen validation error no longer present.

image

DenisHdz
DenisHdz approved these changes Oct 1, 2019
View reviewed changes
Copy link
Contributor

@DenisHdz DenisHdz left a comment

Choose a reason for hiding this comment

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

LGTM

@pjlee11 pjlee11 merged commit cc6e1f9 into latest Oct 2, 2019
@pjlee11 pjlee11 deleted the amp-validation-allowfullscreen-attribute branch October 2, 2019 12:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pjlee11 pjlee11 pjlee11 approved these changes

@dr3 dr3 dr3 approved these changes

@DenisHdz DenisHdz DenisHdz approved these changes

@jamesbhobbs jamesbhobbs Awaiting requested review from jamesbhobbs

@j-pendlebury j-pendlebury Awaiting requested review from j-pendlebury

@jroebu14 jroebu14 Awaiting requested review from jroebu14

@sareh sareh Awaiting requested review from sareh

Assignees

@ibMadbouly ibMadbouly

Labels
ws-media The World Service media stream ws-media- LiveRadioV1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Live radio page fails AMP validation
5 participants
@ibMadbouly @DenisHdz @pjlee11 @dr3 @FK78