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

WORLDSERVICE-413 Audio pages podcast provider links unclickable on desktop #12484

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

Conversation

emilysaffron
Copy link
Contributor

@emilysaffron emilysaffron commented Mar 7, 2025

Resolves JIRA WORLDSERVICE-413

Overall changes

Sets superResponsive to false for audio medialoader

Testing

  1. Visit http://localhost:7080/portuguese/podcasts/p07r3r3t and check that you can click the external links below the media player at every breakpoint

Helpful Links

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

Coding Standards

Repository use guidelines

@emilysaffron emilysaffron self-assigned this Mar 7, 2025
@amoore108
Copy link
Contributor

This does work, however the padding from the media player is still a bit of a problem:
Screenshot 2025-03-07 at 12 03 13

Is there a way to fix this without altering z-index? We should be able to ensure that the media player stays within the bounds of its parent container and not spill over into content outside of it.

@emilysaffron
Copy link
Contributor Author

This does work, however the padding from the media player is still a bit of a problem: Screenshot 2025-03-07 at 12 03 13

Is there a way to fix this without altering z-index? We should be able to ensure that the media player stays within the bounds of its parent container and not spill over into content outside of it.

I can have more of a play around to see, but from what i could tell that padding actually comes from toucan which is why i went to just updating z-index on our wrappers as it seemed easier. Agree the padding is a bit mad though (they also set z-index to be 999 😭)

@amoore108
Copy link
Contributor

amoore108 commented Mar 7, 2025

I can have more of a play around to see, but from what i could tell that padding actually comes from toucan which is why i went to just updating z-index on our wrappers as it seemed easier. Agree the padding is a bit mad though (they also set z-index to be 999 😭)

Yea it likely comes from the media player, although we should be able to override the styling there as they are just HTML elements, rather than being within an iframe (benefit of Toucan!).

I would check things like responsive and superResponsive settings thats were recently changed in the MediaLoader, as they can adjust things like padding.

@karinathomasbbc
Copy link
Contributor

This does work, however the padding from the media player is still a bit of a problem: Screenshot 2025-03-07 at 12 03 13

Is there a way to fix this without altering z-index? We should be able to ensure that the media player stays within the bounds of its parent container and not spill over into content outside of it.

I was going to add - is the recent episodes component not also affected by this, and if we are going with the zIndex solution, do we need to add it to that component too? Or perhaps fixing the container size when in audio skin mode is the right fix, not zIndex?

@emilysaffron
Copy link
Contributor Author

emilysaffron commented Mar 7, 2025

This does work, however the padding from the media player is still a bit of a problem: Screenshot 2025-03-07 at 12 03 13
Is there a way to fix this without altering z-index? We should be able to ensure that the media player stays within the bounds of its parent container and not spill over into content outside of it.

I was going to add - is the recent episodes component not also affected by this, and if we are going with the zIndex solution, do we need to add it to that component too? Or perhaps fixing the container size when in audio skin mode is the right fix, not zIndex?

Yes , the first element in the recent episodes list is unclickable rn ! but adding the z index fix fixed this too - having a play now with changing container size instead of the zindex fix - i would prefer that solution to zindex as it only requires changes in one place! seems a bit cleaner / actually fixing the issue rather than a workaround

@karinathomasbbc
Copy link
Contributor

Once you manage to get this working, would you be able to apply the changes from #12423, so that we can remove force clicking from our tests?

@emilysaffron
Copy link
Contributor Author

Once you manage to get this working, would you be able to apply the changes from #12423, so that we can remove force clicking from our tests?

Sure, i've got it working now - will add the changes from that PR to this one now 👍

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.

3 participants