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

[tvOS] ItemView Button Cleanup #1296

Merged
merged 21 commits into from
Nov 12, 2024
Merged

[tvOS] ItemView Button Cleanup #1296

merged 21 commits into from
Nov 12, 2024

Conversation

JPKribs
Copy link
Member

@JPKribs JPKribs commented Oct 29, 2024

I originally made a branch with some lofty tvOS goal about making completing views and fixing the episode selector. Instead I'm going to settle for a very small change as I dip my toes back into tvOS.

Summary

This PR 'cleans' up the action buttons & the episode cards on ItemViews. More accurately, it's just a bit of style changing for the action buttons but it makes it easier to create new action buttons in the future / change existing action buttons.

My main issue was the focus highlight around the action buttons was both way too big for the button and also a rectangle for a square/circle button selection. So I updated the ActionButton logic into it's own struct and performed all the formatting once for re-use with any additional buttons we may need there (Shuffle, Version Selection?, etc.). Additionally, it used more of the available room for these icons since they were 100x100 but confined to a much smaller area in reality.

I also increased the episode card spacing so focusing wont cause them to overlap anymore. Finally, I changed the play icon I added in a previous PR to be less jarring ( White -> Gray / .Secondary ). I added some spacing to the episode text as well to prevent that being so close to the edges. Finally, some spacing between the rows as well to give them more separation.

Action Button Screenshots

This PR Screenshot 2024-11-08 at 11 26 46
Current Screenshot 2024-11-08 at 11 59 36
trim.1F46241F-E80C-4BF9-A0B7-AFBF1A524071.MOV

Episode Card Screenshots

This PR Screenshot 2024-10-30 at 12 26 12
Current PNG image

@JPKribs JPKribs marked this pull request as draft October 30, 2024 04:07
@JPKribs JPKribs marked this pull request as ready for review October 30, 2024 05:22
…e action buttons to have less extra crap in it
@JPKribs JPKribs changed the title [tvOS] Standardized Action Buttons [tvOS] ItemView Button Cleanup Oct 30, 2024
@JPKribs
Copy link
Member Author

JPKribs commented Oct 31, 2024

Okay, I think this is ready now. I spent a certain amount of time trying to figure out if this should be a viewModifier or a buttonStyle. I settled on a buttonStyle. My thinking is if we have any additional items like this that are borderless buttons, it would be nice if we have a consistent style instead of a) a mix of styles or b) try to recreate this everytime we build something new.

Any feedback is welcome! tvOS and focus are very new to me so I thought this was a good starting point to try and see if I can do custom styling. If there is anything I am doing wrong or needs improvement let me know!

Copy link
Member

@LePips LePips left a comment

Choose a reason for hiding this comment

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

I've always been stumped a bit with these bar buttons since I think just the icons look bare. Having just the icon take color with a large focus view or having the focus clipped to a circle look weird in this area imo. I've thought about making the design for these buttons have a more prominent look that have the background take the primary foreground style instead.

However, now I'm unsure of what to do regarding the saturation of the color with the black icons, as I don't think that looks ideal either.

Screenshot 2024-11-05 at 2 01 59 PM

Take a look at my commit that tests this design, as well as passing in the color through the foregroundStyle instead of as a parameter. I've just recently realized we can wrap things in AnyShapeStyle to intermix different ShapeStyles at the usage site. I will have to go back and replace the color parameters in ToolbarPillButtonStyle.

@JPKribs
Copy link
Member Author

JPKribs commented Nov 6, 2024

I think that looks good! I think my only concern is, as we get more buttons, that might start to get a little overpopulated? I think the current size is good for having the ability to scale, but I agree right now with the two it looks a little bare.

On the other side, do we want to have the action buttons mirror each other between iOS and tvOS? Not necessarily in the focus at all of that but more in the shape and style.

I'm updated this PR to reflect your change since I think that's a lot more native looking. I think this looks awesome with just the 2 buttons but if you add a third:

Screenshot 2024-11-07 at 09 33 12

This starts looking a tad compressed if I were to add #1305 to this. In my mind, this comes down to whether we want more than 2 items (3 with padding changes) there.

This being said, I'm happy with how this looks with your changes and thing this covers all of my concerns for our current iteration! We may need a change down the road if we want delete, shuffle, trailers, or playlists but that's also a lot of buttons. We could just do Play | Favorite | ... then expand that into a menu with other functions? I've added that to my latest commit if you want to take a look and see if this feels like a good direction.

See:

trim.1F46241F-E80C-4BF9-A0B7-AFBF1A524071.MOV

@JPKribs
Copy link
Member Author

JPKribs commented Nov 7, 2024

One revision - Flipped the ellipsis since the that gives us more horizontal spacing:

Screenshot 2024-11-08 at 11 26 46
trim.1F46241F-E80C-4BF9-A0B7-AFBF1A524071.MOV

I've updated the original post with the current version of these changes.

I'm personally pretty happy with how this looks! Let me know if you have any feedback!

Copy link
Member

@LePips LePips left a comment

Choose a reason for hiding this comment

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

as we get more buttons, that might start to get a little overpopulated?

That is something I've had to think about and will have some trouble with. Overall, I think a maximum of 3 actions there is the max but as you have listed there are many more actions that might be appropriate there, with my top 2 being the version selection and trailers.

I don't think that a menu is the best ux choice where we place everything else, as trailers and version selection should be more easily selectable. I also don't want more layers of buttons as that looks busy. So, idk what to entirely do with this area. We could play with widening that area to allow 4 buttons on the second row based on the available content (so that if there's only 2 available actions they won't be so wide).

@JPKribs
Copy link
Member Author

JPKribs commented Nov 9, 2024

I've added these changes to this PR. There is a way to get maybe 1 more on a single row with the ellipsis but it looks like this which might feel a little cramped:

Screenshot 2024-11-09 at 13 19 32

Outside of the menu button, I was thinking something like:

  • Put the buttons in a CollectionHStack that can be scrolled through
    or
  • 2 rows getting us up to 4 to 6 buttons with everything else in a menu button

For scrolling, this is my rough mock up:

Simulator.Screen.Recording.-.Apple.TV.4K.3rd.generation.-.2024-11-09.at.13.53.26.mov

If we do the latter, it might make sense to have a setting for which buttons get included on the screen and which ones are in the menu. But I'm not sure what that looks like on tvOS... Style-wise, I like this PR is but yeah, the scaling side of this is hard to wrap my head around what would look good / make sense.

@LePips
Copy link
Member

LePips commented Nov 11, 2024

Hm, 4 buttons doesn't look the worst and maybe the version selection we can make the small menu you made be on the play button row.

Anyways, this PR good to be merged if the current empty menu is commented out and iOS builds. 👍

@JPKribs
Copy link
Member Author

JPKribs commented Nov 12, 2024

Anyways, this PR good to be merged if the current empty menu is commented out and iOS builds. 👍

I think I have both of these. Please let me know if there are any additional changes I can make!

@LePips LePips enabled auto-merge (squash) November 12, 2024 03:24
@LePips LePips merged commit d276bd7 into jellyfin:main Nov 12, 2024
4 checks passed
@JPKribs JPKribs deleted the tvOSItemView branch November 12, 2024 03:27
@JPKribs JPKribs added the enhancement New feature or request label Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants