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

Implement SSR for amp-audio #523

Merged
merged 6 commits into from
May 19, 2022
Merged

Conversation

ediamin
Copy link
Collaborator

@ediamin ediamin commented May 12, 2022

Fixes #518

This PR adds the SSR support for amp-audio extension. It simply adds <audio controls></audio> element if amp-audio extension is present.

@ediamin ediamin requested a review from westonruter May 12, 2022 06:54
@ediamin
Copy link
Collaborator Author

ediamin commented May 12, 2022

@westonruter I would like to know your opinion about the boilerplate error. Should I remove the error for amp-audio or keep it as it is?

@westonruter
Copy link
Member

I would like to know your opinion about the boilerplate error. Should I remove the error for amp-audio or keep it as it is?

I'm not sure I understand. As I understand, there still should be an error raised for amp-audio if it lacks a child <audio controls style="width:100%;"></audio>, correct?

@westonruter
Copy link
Member

Oh I see. You're adding it if it isn't present originally. In that case, no, the error probably isn't necessary.

@ediamin
Copy link
Collaborator Author

ediamin commented May 13, 2022

Sorry for the confusion. I would like to know your opinion about these two lines. Should I remove them or keep them as it?

$errors->add(Error\CannotRemoveBoilerplate::fromAmpAudio($ampElement));
$canRemoveBoilerplate = false;

ediamin and others added 2 commits May 13, 2022 16:29
Co-authored-by: Weston Ruter <westonruter@google.com>
@ediamin ediamin requested a review from westonruter May 13, 2022 10:46
@westonruter
Copy link
Member

I would like to know your opinion about these two lines. Should I remove them or keep them as it?

It seems to me that they can be removed, no? I don't see any reason why the error would be needed if we can always SSR the amp-audio.

@ediamin
Copy link
Collaborator Author

ediamin commented May 16, 2022

It seems to me that they can be removed, no? I don't see any reason why the error would be needed if we can always SSR the amp-audio.

I'm not sure about that. The comment says, amp-audio requires knowing the dimensions of the browser. Do not remove the boilerplate or apply layout and it is the same in the amp-toolbox node repo.

@westonruter
Copy link
Member

What I mean is that if amp-toolbox-php is always adding the audio child to enable SSR, then there would never be a scenario where the presence of amp-audio would cause any error for SSR. So would the error message ever be correct?

@ediamin
Copy link
Collaborator Author

ediamin commented May 18, 2022

I've updated the code and removed the error for amp-audio.

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

LGTM!

Co-authored-by: Weston Ruter <westonruter@google.com>
@ediamin ediamin merged commit 8fa1588 into main May 19, 2022
@ediamin ediamin deleted the enhance/518-add-amp-audio-ssr-support branch May 19, 2022 09:49
@westonruter westonruter added this to the 0.11.2 milestone Jun 1, 2022
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.

Implement SSR for amp-audio
2 participants