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] ErrorViews - Creation #1414

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

Conversation

JPKribs
Copy link
Member

@JPKribs JPKribs commented Jan 29, 2025

Summary

This is a simplified rework of #1382 that exclusively focuses on the ErrorView. HomeView does behave strangely when it needs to be reloaded but that will need to be handled in a later PR.

This PR cleans up some of the buttons in tvOS to make then better reflect native .card functionality. Additionally, the PrimaryButton is added to tvOS and is used in the new ErrorView. I then mirrored all of the locations in iOS that use the ErrorView and added them to tvOS.

The only unique thing that isn't a direct mirror of iOS is that HomeView will appear next to 2 PagingLibraryViews. Since those views wrap their ErrorView in a ZStack with a Color.xxx above it, there is a vertical offset where the PagingLibraryViews's ErrorView is a tad higher than the HomeView. This isn't an issue it just looks weird. So I added a Color.Clear to the HomeView as well to ensure they're vertically positioned the same. I only added this since you will be able to switch between the HomeView and PagingLibraryViews very quickly so it's a lot more noticeable than if other views have a different position for their ErrorView.


Offset Without Color.Clear

Simulator.Screen.Recording.-.Apple.TV.4K.3rd.generation.-.2025-01-02.at.14.00.36.mp4

ListRowButton for tvOS, I made a change onFocus and onDisabled. onDisabled the buttons should darken 50%. onFocus the buttons would appropriately grow to mirror existing .card buttons but they would stand out. Default .card buttons in tvOS highlight lighter when they are focused while our custom ListRowButton did not. This made the buttons stand out more as custom and also made it less visible to see when they were focused.

ListRowButton Before

Simulator.Screen.Recording.-.Apple.TV.4K.3rd.generation.-.2025-01-29.at.10.25.56.mp4

ListRowButton After

Simulator.Screen.Recording.-.Apple.TV.4K.3rd.generation.-.2025-01-29.at.10.25.10.mp4

ErrorView In Action

Simulator.Screen.Recording.-.Apple.TV.4K.3rd.generation.-.2025-01-29.at.10.28.08.mp4

For reference, here are all of the iOS ErrorView locations. I put the ones that exist in tvOS in red and the ones that ARE tvOS in yellow:

`ErrorView` Locations pre-PR Screenshot 2025-01-29 at 09 53 29

@JPKribs JPKribs added the enhancement New feature or request label Jan 29, 2025
@JPKribs
Copy link
Member Author

JPKribs commented Jan 29, 2025

One another note, while I'm working on these ListRowButton, do we want to switch out the "Sign Out" button? See below:

Current

Simulator.Screen.Recording.-.Apple.TV.4K.3rd.generation.-.2025-01-29.at.10.50.15.mp4

Potential Change

Simulator.Screen.Recording.-.Apple.TV.4K.3rd.generation.-.2025-01-29.at.10.49.20.mp4

@JPKribs JPKribs changed the title [tvOS] ErrorViews [tvOS] ErrorViews - Creation Jan 29, 2025
@LePips
Copy link
Member

LePips commented Jan 29, 2025

Yes, let's please change the Switch User while we're here.

@JPKribs
Copy link
Member Author

JPKribs commented Jan 29, 2025

Yes, let's please change the Switch User while we're here.

I've committed this change! This is what this now looks like.

I changed the ListRowButton to have a maxHeight instead of a flat height so this should reflect its container better. So, this should not be the same size as a standard button (see buttons above and below it). As a result, I need to change the deleteUsersButton to have a height of 75 since this was originally not being set.

Technically, I don't think deleteUsersButton should use ListRowButton and instead use the PrimaryButton but that's a PrimaryButton rework to include role to set it to destructive... for now, deleteUsersButton works and we can adjust that later if we start have other non-list button usages.

Simulator.Screen.Recording.-.Apple.TV.4K.3rd.generation.-.2025-01-29.at.12.51.21.mp4

}
}
.topBarTrailing {
Copy link
Member Author

Choose a reason for hiding this comment

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

I brought this over from iOS. It doesn't seem to do anything and it's always disabled. It makes sense to pull this IMO but, if I'm pulling it from this PR I think it makes sense to pull this from iOS as well? Unless we have a usage for this we want to do in which case I think it should be on both platforms and we should TODO this so we know what we want to do later.

@JPKribs JPKribs requested a review from LePips February 6, 2025 15:52
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