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

PLAYNEXT-1271 Display audio shows and medias in square images for the new PAC Audio page #542

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

Conversation

mutaben
Copy link
Contributor

@mutaben mutaben commented Feb 13, 2025

Description

This is probably the last substantial PR for PAC audio. The rest will be bug fixes and adjustments, unless there's a big change in direction.

The main goal of this PR is to display medias and shows in squared containers.
It was fairly straightforward for Shows since there was already a logic in place to display 2/3 posters.

However for Media it's another story, especially since the associated business object doesn't have any notion of imageVariant, but rather mediaType, and the fact that we have an audio media type doesn't mean we have to automatically display it in a squared container (for instance, in the current audio tab nothing should change and in grid collection the 16/9 aspect must be kept).

Anyway, I think I cracked it, I will try and see if I can make some adjustments, but your reviews are welcome.

If you review commit by commit, there is a part that was rolled back where I tried to adapt imageVariant to medias, but it was a bad idea. Please disregard it.

Changes Made

  • Change feature flag naming to match other similar key (poster) and other platforms
  • Adapt show cell to the new podcast imageVariant
  • Enabled media cell to have different aspect ratios and sizes
  • Adapt media cell to the new audio mediaType
  • Differentiate swimlane and grid cases to decide whether the cell should be squared or rectangular

Checklist

  • I have followed the project's style guidelines.
  • I have performed a self-review of my own changes.
  • I have made corresponding changes to the documentation.
  • My changes do not generate new warnings.
  • I have tested my changes and I am confident that it works as expected and doesn't introduce any known regressions.
  • I have reviewed the contribution guidelines.

@mutaben mutaben added the improvement Feature or update (issue and PR) - release notes section label Feb 13, 2025
Base automatically changed from PLAYNEXT-1964-PLAYNEXT-1634-streamlater-continuelistening-sections-integration to main February 13, 2025 14:34
@mutaben mutaben force-pushed the PLAYNEXT-1271-square-images branch from 028e98c to bd0709c Compare February 13, 2025 15:02
@mutaben mutaben marked this pull request as ready for review February 13, 2025 16:47
@rts-devops rts-devops temporarily deployed to playsrg-tvos-nightly+PLAYNEXT-1271-square-images February 13, 2025 16:51 Inactive
@rts-devops rts-devops temporarily deployed to playsrg-ios-nightly+PLAYNEXT-1271-square-images February 13, 2025 16:52 Inactive
@mutaben mutaben changed the title Playnext 1271 square images PLAYNEXT-1271 Display audio shows and medias in square images for the new PAC Audio page Feb 13, 2025
@rts-devops rts-devops temporarily deployed to playsrg-ios-nightly+PLAYNEXT-1271-square-images February 17, 2025 14:48 Inactive
@rts-devops rts-devops temporarily deployed to playsrg-tvos-nightly+PLAYNEXT-1271-square-images February 17, 2025 14:48 Inactive
@rts-devops rts-devops temporarily deployed to playsrg-tvos-nightly+PLAYNEXT-1271-square-images February 17, 2025 15:32 Inactive
@rts-devops rts-devops temporarily deployed to playsrg-ios-nightly+PLAYNEXT-1271-square-images February 17, 2025 15:33 Inactive
@rts-devops rts-devops temporarily deployed to playsrg-ios-nightly+PLAYNEXT-1271-square-images February 18, 2025 13:04 Inactive
@rts-devops rts-devops temporarily deployed to playsrg-tvos-nightly+PLAYNEXT-1271-square-images February 18, 2025 13:06 Inactive
Copy link
Member

@defagos defagos left a comment

Choose a reason for hiding this comment

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

I had a look at the code but it is difficult for me to provide valuable insights without delving into the implementation and possible alternatives.

A general comment I want to make is that this implementation introduces a lot of surgical layout adjustments, which raises some concerns. I am still asking myself if surgical updates could have been made in less places. Not sure this is possible, though, but without actually doing the work it is quite difficult to say.

Another comment about the layout itself. With the emphasis on show logos as well as the tighter packing of items on screen, I have the impression that the result is more cluttered than what we have today. Of course the data currently displayed might not represent what the result will be after curation, so I might be mislead here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Feature or update (issue and PR) - release notes section
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants