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

feat(MenuFlyout): Add ItemContainerTheme #10020

Conversation

workgroupengineering
Copy link
Contributor

What does the pull request do?

Add ItemContainerTheme to MenuFlyout

What is the current behavior?

MenuFlyout has no ItemContainerTheme

What is the updated/expected behavior with this PR?

How was the solution implemented (if it's not obvious)?

Checklist

Breaking changes

Obsoletions / Deprecations

Fixed issues

Fixes #10010
Depend of #10019 Because it depends on the style fix made in issue #10019

@workgroupengineering workgroupengineering marked this pull request as ready for review January 20, 2023 08:20
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0028956-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

Comment on lines 57 to 58
if ((element as MenuItem)?.ItemContainerTheme == ItemContainerTheme)
element.ClearValue(ItemContainerThemeProperty);
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if I understand.
When I set <MenuFlyout ItemContainerTheme="" /> I would expect same container theme to be applied on flyout items and subitems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you. But I have decided to respect the same behavior of the menus.

protected internal override void PrepareContainerForItemOverride(Control element, object? item, int index)
{
base.PrepareContainerForItemOverride(element, item, index);
// Child menu items should not inherit the menu's ItemContainerTheme as that is specific
// for top-level menu items.
if ((element as MenuItem)?.ItemContainerTheme == ItemContainerTheme)
element.ClearValue(ItemContainerThemeProperty);
}

I've opened a discussion #10060 about it, but no one has responded yet.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's different in case of Menu control.
As in Menu control you have top level menu items that are always rendered horizontally and flyout/popup menus with a different styles. And there you wouldn't expect same style to be applied everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally expected the same behavior also for the Menu as it happens in WPF. I'd like to do a PR that changes the behavior of the menus, but I'm waiting for your response first. I don't want invest time unnecessarily.

@workgroupengineering workgroupengineering force-pushed the features/MenuFlyout/ItemContainerTheme branch from d0d395c to 5123f14 Compare January 25, 2023 08:59
@maxkatz6 maxkatz6 merged commit 30d4644 into AvaloniaUI:master Jan 26, 2023
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0029132-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@workgroupengineering workgroupengineering deleted the features/MenuFlyout/ItemContainerTheme branch January 26, 2023 08:20
@jp2masa jp2masa mentioned this pull request Feb 8, 2023
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.

MenuFlyout ItemTemplate not Applied
3 participants