MenuItem disabled state #1697
MenuItem disabled state #1697
Conversation
IbraheemOsama
added
bug 🐛
| @@ -81,6 +81,13 @@ public MenuItem() | |||
| { | |||
| DefaultStyleKey = typeof(MenuItem); | |||
| IsFocusEngagementEnabled = true; | |||
|
|
|||
| IsEnabledChanged += (s, e) => | |||
There was a problem hiding this comment.
The registration for the event should be in method OnApplyTemplate and also we should unregister from the event as well :) for example you can do
`
///
protected override void OnApplyTemplate()
{
FlyoutButton = GetTemplateChild(FlyoutButtonName) as Button;
_parentMenu = this.FindParent
IsOpened = false;
Items.VectorChanged -= Items_VectorChanged;
IsEnabledChanged -= MenuItem_IsEnabledChanged;
if (MenuFlyout == null)
{
MenuFlyout = new MenuFlyout();
}
else
{
MenuFlyout.Opened -= MenuFlyout_Opened;
MenuFlyout.Closed -= MenuFlyout_Closed;
}
if (FlyoutButton != null)
{
FlyoutButton.PointerExited -= FlyoutButton_PointerExited;
Items.VectorChanged += Items_VectorChanged;
MenuFlyout.Opened += MenuFlyout_Opened;
MenuFlyout.Closed += MenuFlyout_Closed;
FlyoutButton.PointerExited += FlyoutButton_PointerExited;
MenuFlyout.MenuFlyoutPresenterStyle = _parentMenu.MenuFlyoutStyle;
ReAddItemsToFlyout();
IsEnabledChanged += MenuItem_IsEnabledChanged;
if (_isAccessKeySupported)
{
FlyoutButton.AccessKey = AccessKey;
AccessKey = string.Empty;
}
}
UpdateEnabledVisualState();
base.OnApplyTemplate();
}
private void MenuItem_IsEnabledChanged(object sender, DependencyPropertyChangedEventArgs e)
{
var menuItemControl = (MenuItem)sender;
menuItemControl.UpdateEnabledVisualState();
}`
| @@ -13,6 +13,77 @@ | |||
| <commands:NewFileCommand x:Key="NewFile" /> | |||
| <commands:GenericCommand x:Key="GenericCommand" /> | |||
| <converters:StringFormatConverter x:Key="StringFormatConverter" /> | |||
|
|
|||
| <Style TargetType="controls:MenuItem" x:Key="CustomDisabledStateMenuItemStyle"> | |||
There was a problem hiding this comment.
Is this style really needed to demonstrate the functionality of the Menu?
There was a problem hiding this comment.
This style has a minor difference compared to the default Disabled state - mainly the Background color. Default style keeps backwards compatibility by displaying the disabled item with the Buttons disabled background color and the one on the sample sets a transparent background.
There was a problem hiding this comment.
I guess my question is more towards, should we have this style in the sample page? It seems to take up about half the sample, and it doesn't really show the functionality of the Menu, but how to apply a style.
There was a problem hiding this comment.
Sorry, I didn't know this question was for me! What kind of change are we looking at - removing it or improving it (more distinct disabled state)?
There was a problem hiding this comment.
No worries, it was my fault, I should have been more clear. The style does not seem to related to the functionality of the control and I feel it is not necessary to showcase what the control can do. If that is not the case, please feel free to let me know. Otherwise, we should remove the style and keep the sample simple
There was a problem hiding this comment.
Ok, so here's what I think:
- The default
Disabledstyle forMenuItemmimics how it worked before (show the underliyingButtonas disabled) as to not introduce any UI changes. The style in the sample page is added to show that you can customize theDisabledstate as you like, and it will work as expected. - But on a second thought, I think that if someone wanted to style the
Disabledstate they wouldn't need any guidance through this sample, due to already having knowledge on how control templating works and expecting it to just work (that's how I found this bug!).
In the end, I think we can safely remove the change to the sample app and let users know that styling the Disabled state works by themselves.
There was a problem hiding this comment.
Agree 💯 , I'm good with merging this after the style has been removed :)
|
This PR is linked to unclosed issues. Please check if one of these issues should be closed: #1696 |
Issue: #1696
PR Type
What kind of change does this PR introduce?
What is the current behavior?
When setting
IsEnabledto false on aMenuItem, the underlyingButtonuses default values for styling.PR Checklist
Please check if your PR fulfills the following requirements:
What is the new behavior?
MenuItemhas a newDisabledstate that can be styled.Does this PR introduce a breaking change?
Other information