search

ampproject / amp-toolbox-php

AMP Optimizer PHP library
Apache License 2.0
73 stars 25 forks source link

Implement SSR for amp-audio #523

Closed ediamin closed 2 years ago

ediamin commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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

ediamin commented 2 years ago

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;
westonruter commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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