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

Editor: Add support for amp-story-audio-sticker #13540

Merged
merged 56 commits into from
May 23, 2024

Conversation

AnuragVasanwala
Copy link
Collaborator

@AnuragVasanwala AnuragVasanwala commented Dec 21, 2023

Summary

This PR adds support for amp-story-audio-sticker.

User-facing changes

AddAudioSticker.mov
UpdateStickerStyle.mov

Testing Instructions

This PR can be tested by following these steps:

  1. Press the Insert Audio Sticker quick action button. Please note that this button is only visible if the story has video with audio/background audio.
  2. The headphone-cat variant of the sticker is added by default. To change this and other options, select the sticker element and view the style panel.

Reviews

Does this PR have a security-related impact?

Yes.

Does this PR change what data or activity we track or use?

No.

Does this PR have a legal-related impact?

No.

Checklist

  • This PR addresses an existing issue and I have linked this PR to it
  • I have tested this code to the best of my abilities
  • I have verified accessibility to the best of my abilities (docs)
  • I have verified i18n and l10n (translation, right-to-left layout) to the best of my abilities
  • This code is covered by automated tests (unit, integration, and/or e2e) to verify it works as intended (docs)
  • I have added documentation where necessary
  • I have added a matching Type: XYZ label to the PR

Fixes #13440

@swissspidy
Copy link
Collaborator

Whoop whoop, this is really exciting! 🎉 🚀

I'll check it out a bit more closely later.


Possible Bugs in amp-story-audio-sticker:

  1. Clicking/tapping audio sticker doesn't seem to be working in WebStories. Also tried in standalone AMP page and it is > not working. Although, sticker gets toggled when toggling audio from the story (top right button).
  2. On setting the sticker style attribute to outline, the style does not get applied to the sticker. Also tried in standalone AMP page.

This doesn't sound good 👀 So what you are saying is that the audio sticker is actually broken right now? Do you have a link to reproduce or something?

If it's broken, we should pause development on this and ensure it first gets fixed upstream in the AMP repo.

Right click menu for the sticker element.

I can't think of any good right click menu items for this element right now. Let's not add any for now. Products don't have them either.

Style panels other than the default one for the audio sticker element.

What panels are you thinking of adding? The current ones seem to cover all features, no?

Quick actions that appear on selecting an audio sticker element.

Hmm I can't think of any good quick actions for this element. Let's not add any for now. Products don't have quick actions either.

packages/element-library/src/audio-sticker/layer.js Outdated Show resolved Hide resolved
packages/element-library/src/audio-sticker/output.js Outdated Show resolved Hide resolved
packages/element-library/src/constants.ts Outdated Show resolved Hide resolved
packages/element-library/src/elementTypes.js Outdated Show resolved Hide resolved
packages/element-library/src/audio-sticker/display.js Outdated Show resolved Hide resolved
packages/element-library/src/audio-sticker/display.js Outdated Show resolved Hide resolved
packages/element-library/src/audio-sticker/constants.js Outdated Show resolved Hide resolved
@swissspidy swissspidy added Type: Enhancement New feature or improvement of an existing feature Elements: Audio Sticker labels Jan 8, 2024
@AnuragVasanwala

This comment was marked as resolved.

@swissspidy

This comment was marked as resolved.

@swissspidy

This comment was marked as resolved.

@AnuragVasanwala

This comment was marked as resolved.

@swissspidy

This comment was marked as resolved.

@swissspidy

This comment was marked as resolved.

@Swanand01

This comment was marked as resolved.

@swissspidy
Copy link
Collaborator

Fixed them for you.

When importing files like .png in a script file (thanks to Webpack), you need to tell TypeScript about their type.
Also, when importing files in another TS file, they both need to be included by the tsconfig.json. So it's important to add all the necessary files there. Eventually hopefully #13503 converts the whole element-library package to TS, then all files will be included. The last issue was a missing type in getUsedAmpExtensions where you could just look at how the other extensions were defined and properly define the AmpExtension type.

@Swanand01 Swanand01 requested a review from swissspidy April 8, 2024 05:55
@swissspidy
Copy link
Collaborator

@Swanand01 can‘t we do it without sleep?

@Swanand01
Copy link
Collaborator

@Swanand01 can‘t we do it without sleep?

I tried waitFor, but it did not work. This is probably happening because it takes some time for the active element to be set to the text box, after the insert text button is clicked.

@swissspidy
Copy link
Collaborator

@Swanand01 can‘t we do it without sleep?

I tried waitFor, but it did not work. This is probably happening because it takes some time for the active element to be set to the text box, after the insert text button is clicked.

There's rarely a case where waitFor doesn't work. Worst case waitFor also supports a timeout (which is quite high though, 1000ms). I doubt this is such a case. But I also don't want to block this PR. Let's focus on audio stickers for now..

Copy link
Collaborator

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

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

Let's merge this one for now so we can address any follow-ups!

@swissspidy swissspidy merged commit 91460e8 into main May 23, 2024
54 checks passed
@swissspidy swissspidy deleted the feat/13440--amp-story-audio-sticker branch May 23, 2024 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Elements: Audio Sticker Type: Enhancement New feature or improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for <amp-story-audio-sticker>
4 participants