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
fix: recognize ?Cancel/?No as default cancel buttons #26298
Conversation
💖 Thanks for opening this pull request! 💖 We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix. Examples of commit messages with semantic prefixes:
Things that will help get your PR across the finish line:
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can. |
This ignores access keys when determining the default cancelId of a messageBox. This lets e.g. `?Cancel` be recognized as the cancel button.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@camilstaps I'm not sure that I understand what bug this PR fixes. Could you give some more information on what the goal of this PR is? For example, is ?Cancel
a common use case that needs an easy path that can't be handled by options.cancelId
?
Co-authored-by: Charles Kerr <charles@charleskerr.com>
@ckerr thanks for the review! I agree that 'bug' fix may be too harsh, but didn't want to call it a feature either. The documentation for
It seems logical for me that if "Cancel" and "No" are recognized to determine the default, "&Cancel" and "&No" should be recognized as well. This is at least what I would expect reading the documentation, since I read it as if "cancel" and "no" refer to the label visible to the user, i.e., without access keys. You asked:
Yes, I'd say that I can imagine you're afraid of the added complexity(?). In that case maybe the documentation of |
No it's not the complexity I'm concerned about -- I think I may be missing some context about the problem this PR solves, and that's what's confusing to me. When would one want to pass |
Oh, I see the confusion now. |
@ckerr did I need to ping you? Apologies if you were already notified and it simply takes some time—that's fine! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally treating no/cancel as default cancel buttons has been proven a bad idea: it does not work for internationalization, it adds complexity for a very simple thing.
We are not going to remove this feature, but we are also not going to add more complexity for it, so I'm closing this PR.
@zcbenz OK, fair decision. Maybe it's a good idea to make this explicit in the documentation though. |
Description of Change
This allows e.g.
?Cancel
to be recognized automatically as the cancel button.Checklist
npm test
passesRelease Notes
Notes: none