Skip to content

Conversation

@swissspidy
Copy link
Contributor

@swissspidy swissspidy commented Jan 8, 2024

Extracting my question at #39184 (comment) into a PR for more visibility.

Just like <amp-story-shopping-tag>, I don't see why this element shouldn't be allowed to have some wrapper elements. This change would make implementation in Google's WordPress Web Stories plugin more straightforward, where we use absolute positioning for elements.

@swissspidy swissspidy marked this pull request as ready for review January 8, 2024 17:23
@amp-owners-bot amp-owners-bot bot requested a review from dmanek January 8, 2024 17:23
@amp-owners-bot
Copy link

amp-owners-bot bot commented Jan 8, 2024

Hey @ampproject/wg-caching! These files were changed:

extensions/amp-story-audio-sticker/0.1/test/validator-amp-story-audio-sticker-no-grid-layer.out
extensions/amp-story-audio-sticker/validator-amp-story-audio-sticker.protoascii

@swissspidy
Copy link
Contributor Author

/cc @ychsieh

@erwinmombay
Copy link
Member

@banaag could you help review tihs one

@erwinmombay
Copy link
Member

@swissspidy sorry. @ychsieh is currently OOO until next week and I'd like for them to make the final approval

Copy link
Contributor

@ychsieh ychsieh left a comment

Choose a reason for hiding this comment

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

Agree that we should allow wrappers in between. Thanks for the change Pascal!

@swissspidy
Copy link
Contributor Author

@erwinmombay @ychsieh can we merge this?

@ychsieh ychsieh merged commit a2f297f into ampproject:main Jan 30, 2024
@swissspidy swissspidy deleted the patch-1 branch January 30, 2024 17:48
erwinmombay added a commit to erwinmombay/amphtml that referenced this pull request Mar 7, 2024
erwinmombay added a commit to erwinmombay/amphtml that referenced this pull request Mar 7, 2024
eszponder pushed a commit to krzysztofequativ/amphtml that referenced this pull request Apr 22, 2024
…arent (ampproject#39727)

* [amp-story-audio-sticker] make grid layer mandatory ancestor, not parent

* Update out file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants