Skip to content

🖍 [Story audio] Move audio equalizer from video to story system layer#36264

Closed
mszylkowski wants to merge 9 commits intoampproject:mainfrom
mszylkowski:equalizer_tostory
Closed

🖍 [Story audio] Move audio equalizer from video to story system layer#36264
mszylkowski wants to merge 9 commits intoampproject:mainfrom
mszylkowski:equalizer_tostory

Conversation

@mszylkowski
Copy link
Copy Markdown
Contributor

@mszylkowski mszylkowski commented Oct 6, 2021

Currently the audio equalizer is attached to the videos, which makes it very inconsistent for the format:

  • The eq could show up multiple times if there are many videos on the page
  • The eq could be off-screen (not show) if the video is larger than the page (eg: Wordpress Editor stories)
  • The eq doesn't show if the story has a background audio, or the page has audio (becuase it's bound to videos only).

By moving the eq to the story system layer, we ensure there will always be an eq on the page (and only one), properly positioned on the bottom right of the page.

Necessary steps:

  • Remove equalizer from autoplay video if on story
  • Add equalizer to story directly (onto system layer), and manipulate with the store states on MUTED and PAGE_HAS_AUDIO_STATE. This shows the equalizer when there's videos with audio, background-audio for pages, and background-audio for stories.
  • Import styles for equalizer on stories so we don't depend on videos being in the story.
  • For development purposes: Added attribute noaudio on videos in ampconf.html to have a story that shows and hides the equalizer accurately.
Before (cut off)New (positioned in page corner)

Note: Works on RTL but the bars on the eq are not evenly distributed (happens also on main). This PR doesn't change the looks of the equalizer, so I'll leave this bug fix for when the new UI is designed.
image

@mszylkowski mszylkowski changed the title [Story audio] Move audio equalizer from video to story system layer 🖍 [Story audio] Move audio equalizer from video to story system layer Oct 7, 2021
@mszylkowski mszylkowski self-assigned this Oct 7, 2021
@mszylkowski mszylkowski marked this pull request as ready for review October 7, 2021 17:23
@amp-owners-bot amp-owners-bot bot requested a review from nainar October 7, 2021 17:23
@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Oct 7, 2021

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/amp-story-system-layer.js
extensions/amp-story/1.0/amp-story.css
extensions/amp-story/1.0/amp-story.js
extensions/amp-story/1.0/test/test-amp-story-system-layer.js

@mszylkowski mszylkowski removed the request for review from nainar October 7, 2021 17:23
@mszylkowski
Copy link
Copy Markdown
Contributor Author

We plan to remove the audio equalizer altogether so this PR can be closed. Will reopen if we need to reconsider this

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant