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

WSTEAM1-733 Display audio on the Live Page #12031

Open
wants to merge 18 commits into
base: latest
Choose a base branch
from

Conversation

emilysaffron
Copy link
Contributor

@emilysaffron emilysaffron commented Oct 4, 2024

Resolves JIRA WSTEAM1-733
Overall changes

Displays audio within live page posts

Code changes

  • adds audio rendered with the MediaLoader to componentsToRender within Post
  • adds fixture data and unit test

Testing

  1. Visit http://localhost.bbc.com:7081/mundo/live/cemn2qq3x8vt?renderer_env=test and find the post with embedded audio. Observe the working audio player, make sure it looks correct and functions. (Media playing twice is a react re-rendering issue and is expected, it works fine not on local)

Helpful Links

Add Links to useful resources related to this PR if applicable.

Coding Standards

Repository use guidelines

@emilysaffron emilysaffron self-assigned this Oct 4, 2024
@amoore108
Copy link
Contributor

Noticed the background colour inside of a Post, is that ok with UX?
Screenshot 2024-10-04 at 15 40 22

@emilysaffron
Copy link
Contributor Author

Noticed the background colour inside of a Post, is that ok with UX? Screenshot 2024-10-04 at 15 40 22

good question !! will check with them

Comment on lines 169 to 171
expect(
container.querySelector('[data-e2e="media-loader__placeholder"]'),
).toBeInTheDocument();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we need to be more explicit in this test to check the audio SVG is in the page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This now checks for "data-e2e="media-loader" as theres no longer a placeholder for audio - theres no audio SVG to check for anymore either, is specific enough do you think ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it's better, I guess it depends what your trying to test here, are you actually testing for Audio, or are you testing that if there's an audio block it just needs to render the media loader

Copy link
Contributor

@amoore108 amoore108 left a comment

Choose a reason for hiding this comment

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

Looks good 👍 Just a sense check on the other audio player page types, but LGTM.

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.

5 participants