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

[iOS & tvOS] Generic Button to Input from an Alert #1273

Merged
merged 3 commits into from
Oct 15, 2024

Conversation

JPKribs
Copy link
Member

@JPKribs JPKribs commented Oct 15, 2024

I think this is my most re-used component that I was using in #1271 so I thought it made sense to make sure this was done right before I threw this into 20 places.

The goal is to create a generic 'Chevron Button' that displays a value but, when selected, presents an input in an alert. Instead of mandating an input type, I have a trailing 'menu' input option that allows any view to be used as the input type.

Finally, I added some optional logic so this can be Init with strings or text, then the same options with onSave and onCancel logic so this can be used to input values and have those values not take effect until saved.

I wanted to split this out from any real logic changes since I intend on inputting values like this in a few different places so I wanted to be sure this was solid before ended up in a few different locations. For this PR, this is only replacing the maxNextUp input logic in SettingsView > CustomizeViewSettings > Components > HomeSection.

This logic is identical in iOS and tvOS.

Copy link
Contributor

@chickdan chickdan left a comment

Choose a reason for hiding this comment

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

Given the ChevronInputButton.swift is identical for iOS and tvOS it could go in Shared/Components to reduce duplication.

@JPKribs
Copy link
Member Author

JPKribs commented Oct 15, 2024

Given the ChevronInputButton.swift is identical for iOS and tvOS it could go in Shared/Components to reduce duplication.

Good call! Changed.

@LePips
Copy link
Member

LePips commented Oct 15, 2024

Since you were essentially wrapping ChevronButton, I took the opportunity to perform some refactors I had wanted.

Main changes:

  • For cleanup, I was able to bring over my TimeIntervalFormatStyle from my current video player work
  • Rename to ChevronAlertButton, since this at the very least presents an alert and one doesn't even need to implement "input" functionality. However, there is a better name or implementation of this functionality since "Chevron" is a design detail. Perhaps a ViewModifier that creates the TextField and a few other properties like keyboard style and format, since I anticipate this will mostly be for single-form input. At the same time, I anticipate us moving away from alert in many input-contexts.
  • Move ChevronButton to Shared since it's the same view both for iOS/tvOS, probably didn't notice that on initial implementation.
  • Changed the next up day formatting to days for now, as it actually seemed weird to me that you input days and get different date fields back after more use. This may change when we implement field interval input.

Code review:

  • When making views that essentially wrap other views don't copy their implementation as that reduces reusability.
  • When making a few customized inits, please put them below the initial implementation in an extension and try to use the default generated init inside that. You will see that same pattern within most views.

@LePips LePips merged commit 2bda693 into jellyfin:main Oct 15, 2024
4 checks passed
@JPKribs JPKribs deleted the alertInputButton branch October 15, 2024 12:20
@JPKribs JPKribs changed the title Generic Button to Input from an Alert [iOS & tvOS] Generic Button to Input from an Alert Nov 25, 2024
@JPKribs JPKribs added enhancement New feature or request developer Developer facing issues labels Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer Developer facing issues enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants