(iOS) Add support for red cancel button on iOS

BigBalli commented 2 years ago

Platforms affected

Motivation and Context

Description

Testing

Checklist

[ ] I've run the tests to see all new and existing tests pass
[ ] I added automated test coverage as appropriate for this change
[ ] Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
[ ] If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
[ ] I've updated the documentation if necessary

erisu commented 2 years ago

Can you fill out the PR description, following the template?

And if it is to close a ticket also link the ticket. For example: Closes #

Reviewing the code, I have a feeling that this is considered as a breaking change. I feel that it will change the current default visual to red. Anyone who updates a patch or minor and have a visual or behavioral change would not be expected.

Can you please confirm if this is break?

Could there be a flag that determines the visual?

Or is there a way it can be a configuration flag where the supply the color of choice else default to current?

One would expect red is generally the negative action but it is also the users decision to fit their color format..

BigBalli commented 2 years ago

Sorry, first time contributing to a Cordova repo.

I only tested the changes locally, didn't run specific automated tests (not sure what testing environment/workflow you usually use.)
prefixed the commit with platform.
not related to an existing issue
didn't update the documentation as this is indeed "design-breaking".

Please let me know how to proceed, G

On Apr 9, 2022, at 10:40 PM, エリス @.***> wrote:

Can you fill out the PR description, following the template?

And if it is to close a ticket also link the ticket. For example: Closes #

Reviewing the code, I have a feeling that this is considered as a breaking change. I feel that it will change the current default visual to red. Anyone who updates a patch or minor and have a visual or behavioral change would not be expected.

Can you please confirm if this is break?

Could there be a flag that determines the visual?

Or is there a way it can be a configuration flag where the supply the color of choice else default to current?

One would expect red is generally the negative action but it is also the users decision to fit their color format..

— Reply to this email directly, view it on GitHub https://github.com/apache/cordova-plugin-dialogs/pull/157#issuecomment-1094184749, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADIM3XQ2P6CJS5B3OYE6XDVEJSUJANCNFSM5TAD2YKQ. You are receiving this because you authored the thread.

breautek commented 2 years ago

One would expect red is generally the negative action but it is also the users decision to fit their color format..

Agreed. Apple Docs states that the UIAlertActionStyleDestructive style is for:

a style that indicates the action might change or delete data.

In otherwords, you would generally use this style when you have perhaps a delete confirmation prompt, where the OK button is the red / dangerous style.

if([btnLabel isEqualToString:@"Cancel"] || [btnLabel isEqualToString:@"No"]){
    btnStyle=UIAlertActionStyleDestructive;
}

But this PR appears to apply this style to the "cancel" button or a "no" button. With all respect, I think this change is too opinionated and not fit to be in the library. It would be better if we can provide style options so that the developer can opt to choose a style themselves rather than trying to select a style based on the button text.

I understand this might be difficult to accomplish, given that the existing API isn't very extensible.

BigBalli commented 2 years ago

Agreed, very opinionated and would require more work to make it a feature suitable for everyone.

G

On Apr 10, 2022, at 2:12 PM, Norman Breau @.***> wrote:

One would expect red is generally the negative action but it is also the users decision to fit their color format..

Agreed. Apple Docs https://developer.apple.com/documentation/uikit/uialertactionstyle/uialertactionstyledestructive states that the UIAlertActionStyleDestructive style is for:

a style that indicates the action might change or delete data.

In otherwords, you would generally use this style when you have perhaps a delete confirmation prompt, where the OK button is the red / dangerous style.

if([btnLabel isEqualToString:@"Cancel"] || [btnLabel isEqualToString:@"No"]){ btnStyle=UIAlertActionStyleDestructive; } But this PR appears to apply this style to the "cancel" button or a "no" button. With all respect, I think this change is too opinionated and not fit to be in the library. It would be better if we can provide style options so that the developer can opt to choose a style themselves rather than trying to select a style based on the button text.

I understand this might be difficult to accomplish, given that the existing API isn't very extensible.

— Reply to this email directly, view it on GitHub https://github.com/apache/cordova-plugin-dialogs/pull/157#issuecomment-1094370427, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADIM3TPSJAOZDXDXAJ5AOLVEM74VANCNFSM5TAD2YKQ. You are receiving this because you authored the thread.

jcesarmobile commented 1 year ago

Going to close since it's breaking and opinionated UIAlertActionStyleDestructive should be used for destructive actions but it's being used for Cancel and No, I think UIAlertActionStyleCancel would suit those buttons better.

not sure if it would be possible to modify the plugin to allow setting buttons as strings as it does now but also as objects where you can pass the string and the style, so that way is not breaking

or deprecating "buttonLabels" and adding "buttons" that would take the object with label and style.

apache / cordova-plugin-dialogs