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

Add skipOption to AlertType #349

Closed

Conversation

kalupas226
Copy link

Nice to meet you!
Thank you for letting us use your wonderful library!

Today, I want to use AlertController with only skip and update action.
So I send you a pull request.

If you have free time, please check my pull request?

@kalupas226
Copy link
Author

And sorry, I tried to edit html docs.
But I wasn't sure where to edit, so I decided to ask your opinion.
Best regards.

@ArtSabintsev
Copy link
Owner

Hi,

Thanks for the PR!

The HTML docs are automatically generated using jazzy. So in 8 years of managing Harpy and Siren, no one has ever asked for this feature. What's the use case here?

Thanks!

@kalupas226
Copy link
Author

kalupas226 commented Sep 24, 2020

@ArtSabintsev
Thanks for your reply!

The HTML docs are automatically generated using jazzy.

I see. Thank you.

no one has ever asked for this feature

AlertType.option has two alerts: nextTimeAlertAction and updateAlertAction.
NextTimeAction will alert user again after the next launch.
However, I thought that to be a bit noisy for some people.
Also, AlertType.skip has three actions.
However, I thought the number of actions to be redundant for the situations I wanted to use it for.
Also, I wanted to keep the number of buttons in the Alert to two in accordance with the Human Interface Guideline, even though there are three buttons displayed in the Alert.

So, I thought I want to use two button alert has skip and update button.

Thanks!

@ArtSabintsev
Copy link
Owner

ArtSabintsev commented Sep 25, 2020

Hey, I thought this over. While I am principally not against it, I don't think it's necessary. This is the first time in 8 years I have received this request, which to me, means that it's a feature for the minority of users (and/or developers), and if anything, adds to the complexity of configuration of Siren by adding a feature that will be used by a minority of folks (if they even discover it).

While a 3 button UIAlertController may not align with the HIG, it's still allowed by virtue of the UI being able to support it. Also, Apple has introduced their own AlertController's with 3 interaction points, so they're not following their own suggested rules to some extent.

While I appreciate the sentiment here, and that you care about noisiness for some people, as you called it, I don't see that as being worthwhile enough to add to the library. I tend to write code for the majority of use cases, not the minority, especially when vendoring an open source library.

For all the reasons above, I will be declining this PR. You are welcome to use it on your own fork for your own needs, especially as Siren is in a very stable state these days, so there shouldn't be too many times where you'd need to sync with master.

Thanks for understanding!

@kalupas226
Copy link
Author

@ArtSabintsev

Thanks for taking the time to think about it.
And thank you for your careful explanation, I was able to agree with your opinion.

I certainly think Siren is so stable that I decided to either rethink the design of the app or fork Siren over to use it.

Thank you!!

@ArtSabintsev
Copy link
Owner

Awesome - happy to hear it. I'll be locking this thread now

Repository owner locked as resolved and limited conversation to collaborators Sep 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants