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] Documentation and validator tests #39184

Merged
merged 76 commits into from
Aug 1, 2023

Conversation

ychsieh
Copy link
Contributor

@ychsieh ychsieh commented Jun 21, 2023

No description provided.

@edoudou
Copy link
Contributor

edoudou commented Jul 21, 2023

@erwinmombay @mszylkowski when do you expect the widget to be compliant ?

}
attrs: {
name: "sticker"
value: "cat-sticker"
Copy link
Contributor

Choose a reason for hiding this comment

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

This value looks like it duplicates the "sticker" naming twice, in the attr and value. Should this be just "cat"? The other values don't say sticker at the end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dig a bit deeper into the UX mock. You are right the real name is headphone-cat : ) Apologies to the cat 🐱

# Allowlisting as many components as possible, unless they could result in a
# bad UX.
name: "amp-story-audio-sticker-allowed-descendants"
tag: "A"
Copy link
Contributor

Choose a reason for hiding this comment

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

This list is way too permissive. We should only allow amp-img, anything else?

I'm worried if people add links or interactive components here, as the behavior is not desired.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure we can make it less permissive. Let's also add div and span for CSS-only implementation(no extra network requests) of the sticker. Anything you want to add from that perspective?

@ychsieh ychsieh requested a review from erwinmombay July 26, 2023 20:05
@ychsieh ychsieh self-assigned this Jul 26, 2023
@swissspidy
Copy link
Contributor

@ychsieh Curious, when is the validator going to be updated accordingly? I don't see this yet at https://cdn.ampproject.org/v0/validator.json

@ychsieh
Copy link
Contributor Author

ychsieh commented Sep 5, 2023

It's in progress and will be out in this or next week.

@swissspidy
Copy link
Contributor

@ychsieh Another question, as we're now looking into adding support for this to the Google Web Stories WordPress editor. What's the reason amp-story-audio-sticker must be a direct descendant of amp-story-grid-layer? This makes it really difficult to position the element differently, as we normally have a couple of wrapper divs and use absolute positioning for elements (instead of relying on amp-story-grid-layer templates). Can this be made less strict?

eszponder pushed a commit to krzysztofequativ/amphtml that referenced this pull request Apr 22, 2024
…roject#39184)

* Audio sticker skeleton code: clicking on the element would unmute the story and hide the element

* Remove validator test and spaces in css file

* Add dependency

* Update z-index

* Update extensions/amp-story-audio-sticker/0.1/amp-story-audio-sticker.css

Co-authored-by: Matias Szylkowski <mszylkowski@google.com>

* Cleaning up extension and css code

* fix

* Remove unnecessary CSS and update the example.

* Fix z-index

* Remove validator

* Add the sticker container and tap hint

* Fix whitespace

* lint

* String concat in jsx

* Default stickers and switching between different states

* Remove duplicate buildCallback

* Use CSS to execute animation delay instead. Have a new helper function to get default sticker

* Change default size to small

* Fix lint

* Fix unit test

* refactors

* Move amp-img src setting to buildcallback

* Fixes

* Fix

* Update extensions/amp-story-audio-sticker/0.1/amp-story-audio-sticker.js

Co-authored-by: Matias Szylkowski <mszylkowski@google.com>

* lint

* Add animation and styles to sticker

* Rename css variable

* Replace borders with box-shadow.
Use sticker-style instead.
Replace mouse and touch events with platform-agnostic events: pointerdown and pointerup

* Fix unit test and merge with main.

* Replace unnecessary classes and event listeners with CSS selectors.
Remove RGB color checks.

* Allow developers to set the CSS variables directly,
instead of taking it as element attributes and do the checks

* Replace box-shadow with filter drop-shadow
This is to create a  dropshadow that conforms to the shape.

* Ensure transitions work on both scaling down and up.

* minor fixes

* Make color variables global.
Combine two animations into one.

* Remove comment

* Use inset: 0 instead.

* Highlight mute button only when the sticker is clicked

* unit test

* Get system UI in the click handler instead of layout callback to prevent system UI from not built yet.
Remove SYSTEM_UI_IS_BUILT state from store service

* Trivial simplification

* Update extensions/amp-story/1.0/amp-story-system-layer.css

Co-authored-by: Matias Szylkowski <mszylkowski@google.com>

* Update extensions/amp-story-audio-sticker/0.1/amp-story-audio-sticker.js

Co-authored-by: Matias Szylkowski <mszylkowski@google.com>

* Fix lint

* Sticker documentation and validator

* Try validator

* Update with validator-generated .out file.

* Manually fix validator

* Simplify validators
Show premade effects as tables in documentation

* Remove player tags

* Fix validator rules

* Change sticker name and disallow most child tags

* Fix unit test

* Add validator test case for not in grid layer

---------

Co-authored-by: Matias Szylkowski <mszylkowski@google.com>
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.

6 participants