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] Add pin prompt to sign-in screen #1383

Merged
merged 14 commits into from
Jan 9, 2025
Merged

Conversation

chickdan
Copy link
Contributor

@chickdan chickdan commented Jan 2, 2025

Most of this is brought over from iOS with little changed. I removed LocalAuthentication since the framework does not support tvOS so the security options right now are none or pincode.

One oddity I noticed that seems specific to tvOS is that TextField with placeholder text will add an additional button to alerts that have the same text as the placeholder. Clicking the button opens the keyboard but it's a bit odd so I'm not sure if that is desired to be kept in there.
Example:

Screenshot 2025-01-08 at 12 33 24 AM

I also left in the password reset and reset settings buttons (but commented out) for later use if it's desired to include that functionality.

Set pin

set_pin.mp4

Attempt Login

pin_login.mov

@chickdan chickdan changed the title WIP [tvOS] Add pin prompt to sign-in screen [tvOS] Add pin prompt to sign-in screen Jan 8, 2025
@chickdan chickdan marked this pull request as ready for review January 8, 2025 06:29
chickdan and others added 2 commits January 8, 2025 00:30
- Move UserProfileSettings to their own Coordinator
- Make Views Modal to better reflect existing items
- Fix CustomizeSettingsCoordinator (This is on me!)
- Change PINs to use SecureField
- Move all Settings View to use SplitFormWindowView to mirror existing Settings
- Use user profile image for SplitFormWindowView Icon
- Change Profile Security to use LearnMoreModal
- Use suggestion from https://forums.developer.apple.com/forums/thread/739545
- Tag Alert > TextFields with TODO so we can check this on tvOS 18
@JPKribs
Copy link
Member

JPKribs commented Jan 8, 2025

I am so sorry, @chickdan! I didn't realize those would just direct commit I thought those would send you a PR to your local branch! You are totally welcome to revert those!

Here is what I wanted to send as changes. I'm sure @LePips will have a much better insight but just from my prior tvOS PRs:

  • Move UserProfileSettings to their own Coordinator

    • I did this just to help clean it out a little. It's not a requirement by any stretch but as we add other UserProfileSettings it would help keep things from getting cluttered.
  • Make Views Modal to better reflect existing items

    • The other Settings & CustomizeSettings use Modal instead of Push. Just mirroring those.
  • Fix CustomizeSettingsCoordinator (This is on me!)

    • This was just an issue I accidentally where I was treating this as BasicNavigationViewCoordinator when it should have been CustomizeSettingsViewCoordinator. Changing this resolves a routing issue I created but was not aware of until now.
  • Change PINs to use SecureField

    • Just a preference. I'd be interested to hear everyone else's take on this!
  • Move all Settings View to use SplitFormWindowView to mirror existing Settings

    • All Settings are SplitFormWindowView with an icon on the left. This just moves those over to this format to be a little more cohesive.

Simulator Screenshot - Apple TV 4K (3rd generation) - 2025-01-08 at 08 50 34

Simulator Screenshot - Apple TV 4K (3rd generation) - 2025-01-08 at 08 50 40

  • Use user profile image for SplitFormWindowView Icon

    • iOS we have the profile image at the top. I used SplitFormWindowView to put it on the side. IMO, it looks good but open to feedback!
  • Change Profile Security to use LearnMoreModal

    • We've moved a few longer descriptions over to this layout. With this change, I'm not sure the text lines up exactly now but it de-clutters the list on the right now.
  • Hide Device Authentication option

    • Either this setting is not possible or maybe we are able to assign this to a tvOS Profile in the future? For now, I think Pin or None are our only options so this makes sense as an inline toggle instead of another view selection.
  • Use suggestion from https://forums.developer.apple.com/forums/thread/739545

    • In trying to find a good article on this, I found this! It looks like this resolves the issue.
      Simulator Screenshot - Apple TV 4K (3rd generation) - 2025-01-08 at 09 11 55
  • Tag Alert > TextFields with TODO so we can check this on tvOS 18

    • In the future, placeholder text would be nice so I left TODOs for us to re-enable this on tvOS and hopefully it's resolved!

Sorry again for the commit over! Please let me know if you have any questions!

@chickdan
Copy link
Contributor Author

chickdan commented Jan 8, 2025

@JPKribs no worries! I leave the "allow maintainers to make changes" box selected for this very reason 🙂 these new designs look SO MUCH better! These two PRs are my first foray into tvOS so I am still very much unfamiliar with design standards so this is very helpful, thank you! I'll look into the code changes and respond to the rest of your comments tonight after work.

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.

Thank you for implementing this @chickdan and thank you for the changes @JPKribs. tvOS "design" is still pretty dynamic as we figure out better UX interactions on the platform. For example, we may want to change the Save button from the navigation bar into a button in the list, similar to Change User.

@LePips LePips merged commit f9ebebe into jellyfin:main Jan 9, 2025
4 checks passed
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