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

RadioMenuFlyoutItem API review #19

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

MikeHillberg
Copy link
Contributor

No description provided.

@MikeHillberg MikeHillberg requested a review from a team as a code owner March 13, 2019 20:27
@MikeHillberg MikeHillberg removed the request for review from a team March 13, 2019 20:29
@adambarlow
Copy link
Contributor

Looks great. I approve.

Boolean IsChecked;

/// Gets or sets the name that specifies which RadioMenuFlyoutItem controls are mutually exclusive
String GroupName;
Copy link
Member

@oldnewthing oldnewthing Mar 15, 2019

Choose a reason for hiding this comment

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

I assume the GroupName scope is the containing MenuFlyout? Or is it scoped to the MenuFlyoutSubItem if in a subitem?

Copy link
Member

Choose a reason for hiding this comment

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

It's scoped to siblings in whatever flyout it's in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Recommend: Need to update this in the description.

# API Details
````c#
[webhosthidden]
unsealed runtimeclass RadioMenuFlyoutItem : Windows.UI.Xaml.Controls.MenuFlyoutItem
Copy link
Member

Choose a reason for hiding this comment

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

RadioButton derives from ToggleButton, but RadioMenuFlyoutItem doesn't derive from ToggleMenuFlyoutItem. Intentional?

Copy link
Member

Choose a reason for hiding this comment

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

We did do that intentionally but I could be convinced otherwise. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does/should it support ThreeState?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it should, that doesn't feel like a compelling scenario inside a context menu.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussion: Based on that, not deriving from ToggleMenuFlyoutItem sounds right.

@YuliKl
Copy link
Contributor

YuliKl commented May 14, 2019

This API was released in WinUI 2.1, so I beleive this PR is no longer necessary to keep active.

@YuliKl YuliKl closed this May 14, 2019
@jevansaks
Copy link
Member

Doesn't this still need to be merged into master? @MikeHillberg

@YuliKl
Copy link
Contributor

YuliKl commented May 14, 2019

I believe this PR was the API board's copy. The PR I had created, that got API board approval, incorporates this feedback and is checked in here: https://github.com/microsoft/microsoft-ui-xaml-specs/tree/master/active/RadioMenuFlyoutItem

But I may be confused about this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants