You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@cordova.apache.org by GitBox <gi...@apache.org> on 2022/04/10 02:22:39 UTC
[GitHub] [cordova-plugin-dialogs] BigBalli opened a new pull request, #157: Add support for red cancel button on iOS
BigBalli opened a new pull request, #157:
URL: https://github.com/apache/cordova-plugin-dialogs/pull/157
<!--
Please make sure the checklist boxes are all checked before submitting the PR. The checklist is intended as a quick reference, for complete details please see our Contributor Guidelines:
http://cordova.apache.org/contribute/contribute_guidelines.html
Thanks!
-->
### Platforms affected
### Motivation and Context
<!-- Why is this change required? What problem does it solve? -->
<!-- If it fixes an open issue, please link to the issue here. -->
### Description
<!-- Describe your changes in detail -->
### Testing
<!-- Please describe in detail how you tested your changes. -->
### 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](https://help.github.com/articles/closing-issues-using-keywords/))
- [ ] I've updated the documentation if necessary
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-plugin-dialogs] erisu commented on pull request #157: Add support for red cancel button on iOS
Posted by GitBox <gi...@apache.org>.
erisu commented on PR #157:
URL: https://github.com/apache/cordova-plugin-dialogs/pull/157#issuecomment-1094184749
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 #<ticket number>
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..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-plugin-dialogs] BigBalli commented on pull request #157: (iOS) Add support for red cancel button on iOS
Posted by GitBox <gi...@apache.org>.
BigBalli commented on PR #157:
URL: https://github.com/apache/cordova-plugin-dialogs/pull/157#issuecomment-1094372245
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.
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-plugin-dialogs] jcesarmobile commented on pull request #157: (iOS) Add support for red cancel button on iOS
Posted by "jcesarmobile (via GitHub)" <gi...@apache.org>.
jcesarmobile commented on PR #157:
URL: https://github.com/apache/cordova-plugin-dialogs/pull/157#issuecomment-1463040947
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-plugin-dialogs] BigBalli commented on pull request #157: Add support for red cancel button on iOS
Posted by GitBox <gi...@apache.org>.
BigBalli commented on PR #157:
URL: https://github.com/apache/cordova-plugin-dialogs/pull/157#issuecomment-1094368325
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.
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-plugin-dialogs] breautek commented on pull request #157: (iOS) Add support for red cancel button on iOS
Posted by GitBox <gi...@apache.org>.
breautek commented on PR #157:
URL: https://github.com/apache/cordova-plugin-dialogs/pull/157#issuecomment-1094370427
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-plugin-dialogs] jcesarmobile closed pull request #157: (iOS) Add support for red cancel button on iOS
Posted by "jcesarmobile (via GitHub)" <gi...@apache.org>.
jcesarmobile closed pull request #157: (iOS) Add support for red cancel button on iOS
URL: https://github.com/apache/cordova-plugin-dialogs/pull/157
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org