[Windows/macOS] Implement menu key accelerators #14740
Conversation
jsuarezruiz
added
t/bug
|
We need to do the same on macOS. It will be in a separate PR. |
There was a problem hiding this comment.
LGTM! Let's hold off on merging until we have a better sense of the Mac implementation though, to ensure this API looks the way we want it to.
Things we might consider/look more into:
- Separating the modifier and key into separate paramaters, and not using strings (especially with parsing around
+) - Create a property to enable implementation via XAML
- Tests
src/Controls/samples/Controls.Sample/Pages/Core/ContextFlyoutPage.xaml.cs
Show resolved
Hide resolved
|
@jknaudt21 I added a couple for now, but not all. I noticed we don't have any for any of the extensions or mappers in Core and wondering if that's intentional on our team's part. I'm also not sure if we have any public-facing docs for Core APIs? was only able to find Controls docs |
|
Updated the |
Added pending docstrings to the public APIs. |
| @@ -145,6 +145,9 @@ static object CoerceIsEnabledProperty(BindableObject bindable, object value) | |||
| return false; | |||
| } | |||
|
|
|||
| IReadOnlyList<IAccelerator> IMenuElement.Accelerators => | |||
| GetAccelerator(this) is Accelerator acc ? new[] { acc } : null; | |||
There was a problem hiding this comment.
Same comment, should we one returning null? Or an empty list of IAccelerator. Why the need of a IReadOnlyList? Are users going to access the by index?
There was a problem hiding this comment.
The order matters I think because you want the accelerators to be in a specific order. The null is not my favorite but some people like it - not sure it matters. We can always not-null it later, but making it null later is breaking.
|
|
||
| var keyboardAccelerator = new KeyboardAccelerator(); | ||
| keyboardAccelerator.Key = key.ToVirtualKey(); | ||
| if (modifiers is not null) |
There was a problem hiding this comment.
Making modifiers to be an empty array rather than nullable will remove this type of checks.
| if (int.TryParse(key, out int numberKey)) | ||
| { | ||
| if (numberKey >= 0 && numberKey <= 9) | ||
| { | ||
| key = $"Number{key}"; | ||
| } | ||
| } |
There was a problem hiding this comment.
Exactly, so the setup is wrong, yet we are not throwing an exception or anything, meaning that we will have an issue at run time (key is revamped maybe?). If the number is out of range, we should throw and out of range exception.
src/Core/src/Handlers/MenuFlyoutSubItem/MenuFlyoutSubItemHandler.Windows.cs
Show resolved
Hide resolved
src/Core/src/Handlers/MenuFlyoutItem/MenuFlyoutItemHandler.Windows.cs
Outdated
Show resolved
Hide resolved
src/Core/src/Handlers/MenuFlyoutSubItem/MenuFlyoutSubItemHandler.Windows.cs
Show resolved
Hide resolved
…dows.cs Co-authored-by: Juan Diego Herrera <juherrera@microsoft.com>
Description of Change
Implement menu key accelerators.
Issues Fixed
Fixes #5211
Related with #14021