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

Add support for <amp-story-audio-sticker> #13440

Open
swissspidy opened this issue Aug 2, 2023 · 3 comments · Fixed by #13540
Open

Add support for <amp-story-audio-sticker> #13440

swissspidy opened this issue Aug 2, 2023 · 3 comments · Fixed by #13540
Assignees
Labels
AMP Format Issues that need to be addressed in the AMP story format AMP Output Issues related to AMP output and validation Group: Workspace A group encompassing all of workspace – everything inside the canvas and sidebar P2 Should do soon Type: Enhancement New feature or improvement of an existing feature
Milestone

Comments

@swissspidy
Copy link
Collaborator

swissspidy commented Aug 2, 2023

Feature Description

This new amp-story-audio-sticker extension has been added recently, see ampproject/amphtml#39184

245924556-e953c54a-0a09-4f15-8134-be6a0ac758ab.mp4

No limits in how many stickers can be added to a story.

Color of the sticker drop shadow and outline can be customized.

Stickers can be added in two different sizes:

  • “large”: 180 x 180 px
  • “small”: 120 x 120 px

The following stickers exist:

Accepted Value Pre-tap Image Post-tap Image
“headphone-cat” Cat Pre-tap Cat Post-tap
“tape-player” Player Pre-tap Player Post-tap
“loud-speaker” Speaker Pre-tap Speaker Post-tap
“audio-cloud” Cloud Pre-tap Cloud Post-tap

Alternatives Considered

Additional Context

Currently blocked by:

@swissspidy swissspidy added Type: Enhancement New feature or improvement of an existing feature Status: Blocked On hold for the time being AMP Format Issues that need to be addressed in the AMP story format AMP Output Issues related to AMP output and validation P3 Nice to have Group: Workspace A group encompassing all of workspace – everything inside the canvas and sidebar labels Aug 2, 2023
@swissspidy swissspidy changed the title Add support for <amp-story-audio-sticker> Add support for <amp-story-audio-sticker> Aug 2, 2023
@swissspidy
Copy link
Collaborator Author

From chatting with @AnuragVasanwala:

  • 3p media tab probably not a good location, as that's not where a user would expect to find the audio stickers
  • stickers/shapes tab might be a more suitable place
  • how can we make users aware of this new feature?

@swissspidy swissspidy removed the Status: Blocked On hold for the time being label Nov 15, 2023
@swissspidy
Copy link
Collaborator Author

swissspidy commented Nov 22, 2023

Some thoughts on implementation details:

packages/element-library

  • Register a new audio-sticker element, which will be responsible for displaying the sticker in the editor and generating the actual <amp-story-audio-sticker> markup for viewing the story
  • Supports the following attributes:
    • size: 'small', 'large' (small is default)
    • sticker: 'headphone-cat','tape-player','loud-speaker','audio-cloud' (headphone-cat is default)
    • style: 'outline','dropshadow', (default is none)
  • Needs unit tests, including for AMP validation to ensure the output generated by output.js is valid AMP
  • Look at product element type for inspiration, as it's the most recent one.
  • Attributes in constants.js are especially important, need to see which ones make sense for this element type.
  • Might be a good opportunity to first convert the element-library package to TypeScript

packages/output

  • Add some tests with audio-sticker elements to verify that the sticker is properly added to story output and markup is valid AMP
  • Might be a good opportunity to convert the output package to TypeScript

KSES.php

  • Add the new elements to the allowlist to ensure these stickers won't be stripped when saving a story as an author or contributor user
    • Can be verified with e2e tests later
  • See https://github.com/ampproject/amp-wp/pull/7603/files for the structure
  • Update PHPUnit tests accordingly

In the editor UI

@swissspidy swissspidy added this to the 1.36.0 milestone Nov 24, 2023
@swissspidy
Copy link
Collaborator Author

From our discussion just now:

  • Starting with just the 4 default stickers. Maybe in the future provide option to upload a custom image or provide a richer library of default stickers
  • Let's test whether the stickers can be animated
  • In the future we can add a checklist item for non-ideal placement of the sticker (e.g. when it is on a page after a page with audio, or when there are too many stickers on a page / in a story.
  • Not limiting the number of stickers for now
  • Let's put all the panels into the sidebar for now (as per above) and then later look at what to put in the floating menu as well.

To detect whether a story has any audio (untested but should work):

  const hasAudioAnywhere = useStory(
    ({ state }) =>
      state.story.backgroundAudio ||
      state.pages.some(
        (page) =>
          page.backgroundAudio ||
          page.elements
            .filter(elementIs.video)
            .some((element) => !element.resource.isMuted)
      )
  );

@swissspidy swissspidy modified the milestones: 1.36.0, 1.37.0 Apr 4, 2024
@swissspidy swissspidy added P2 Should do soon and removed P3 Nice to have labels May 6, 2024
@swissspidy swissspidy reopened this May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AMP Format Issues that need to be addressed in the AMP story format AMP Output Issues related to AMP output and validation Group: Workspace A group encompassing all of workspace – everything inside the canvas and sidebar P2 Should do soon Type: Enhancement New feature or improvement of an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants