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-story-audio-sticker] make grid layer mandatory ancestor, not parent #39727

Merged
merged 3 commits into from
Jan 30, 2024
Merged

🐛 [amp-story-audio-sticker] make grid layer mandatory ancestor, not parent #39727

merged 3 commits into from
Jan 30, 2024

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
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
42 checks passed
@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