Skip to content
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

Add check to only open flyout if a menubaritem has items #2889

Merged
merged 3 commits into from
Jul 17, 2020

Conversation

marcelwgn
Copy link
Collaborator

Description

Adds a check to only open a menubaritems flyout if it has items.

Motivation and Context

Closes #2740

How Has This Been Tested?

Tested manually:

gif

Screenshots (if appropriate):

@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Jul 12, 2020
@ranjeshj ranjeshj added team-Controls Issue for the Controls team area-Menus labels Jul 13, 2020
@@ -55,6 +55,8 @@
<MenuFlyoutItem Text="Word Wrap"/>
<MenuFlyoutItem Text="Font..."/>
</muxc:MenuBarItem>

<muxc:MenuBarItem Title="No children"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @chingucoding. Could we add a test ? I think you can click and look at all the open popups to validate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added new interaction test now.

[TestMethod]
public void EmptyMenuBarItemNoPopupTest()
{
if (PlatformConfiguration.IsDevice(DeviceType.Phone))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need that? I don't think we are doing any tests on Windows Mobile devices any longer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the same. I think we should look at all test holistically at some point and remove obsolete checks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does look like dead code. @kmahone to verify this codepath is not hit in any of the configurations we run tests in.

Copy link
Contributor

@StephenLPeters StephenLPeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters StephenLPeters removed the needs-triage Issue needs to be triaged by the area owners label Jul 15, 2020
@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters StephenLPeters merged commit 0a757bd into microsoft:master Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Menus team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MenuBarItem with no Items shows blank Dropdown
5 participants