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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 9 commits into from

Conversation

mszylkowski
Copy link
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 added this to In progress in wg-stories Sprint via automation 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

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
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
Development

Successfully merging this pull request may close these issues.

None yet

1 participant