Skip to content
This repository was archived by the owner on Feb 25, 2026. It is now read-only.

Menu: changed MenuItem Header to type object - #1294

Merged
nmetulev merged 4 commits into
devfrom
menuitemheader
Jul 12, 2017
Merged

nmetulev merged 4 commits into
devfrom
menuitemheader

Conversation

@nmetulev

@nmetulev nmetulev commented Jul 5, 2017

Copy link
Copy Markdown
Contributor

re #1293, allows having other content than string inside a MenuItem

@nmetulev nmetulev added this to the v2.0 milestone Jul 5, 2017
@nmetulev
nmetulev requested a review from IbraheemOsama July 5, 2017 21:32
/// Identifies the <see cref="Header"/> dependency property.
/// </summary>
public static readonly DependencyProperty HeaderProperty = DependencyProperty.Register(nameof(Header), typeof(string), typeof(MenuItem), new PropertyMetadata(default(string)));
public static readonly DependencyProperty HeaderProperty = DependencyProperty.Register(nameof(Header), typeof(object), typeof(MenuItem), new PropertyMetadata(null));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@nmetulev Why object instead of string ? :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This way you can do something like

<controls:MenuItem Name="FileMenu"
                   controls:Menu.InputGestureText="Alt+F">
    <controls:MenuItem.Header>
        <StackPanel Orientation="Horizontal">
            <SymbolIcon Symbol="Accept"></SymbolIcon>
            <TextBlock Text="Hello"></TextBlock>
        </StackPanel>
    </controls:MenuItem.Header>
</controls:MenuItem>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Because of content is object, I looked through the code

@skendrot skendrot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should have Header and HeaderTemplate

@skendrot skendrot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not just make it a ContentControl?

@IbraheemOsama

Copy link
Copy Markdown
Member

@skendrot we can't because it is already an ItemsControl

@skendrot

skendrot commented Jul 5, 2017

Copy link
Copy Markdown
Contributor

Good point!

@skendrot

skendrot commented Jul 5, 2017

Copy link
Copy Markdown
Contributor

Should still allow to override template

@IbraheemOsama

Copy link
Copy Markdown
Member

I agree, we need to add new property HeaderTemplate and bind it to the Button template in the menu

@skendrot skendrot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the Header be changed or add a property to show icons?

@IbraheemOsama

Copy link
Copy Markdown
Member

No icons please :) if he needs he can override.

@nmetulev

nmetulev commented Jul 6, 2017

Copy link
Copy Markdown
Contributor Author

IMO, The MenuItem can always be re-styled to change the header template.

@tibitoth

tibitoth commented Jul 6, 2017 •

Copy link
Copy Markdown
Contributor

Sorry I missed that MenuFlyoutItem has Icon property in #1293. MenuItem does not need to have Icon by default.

@IbraheemOsama

IbraheemOsama commented Jul 8, 2017 •

Copy link
Copy Markdown
Member

I'm with adding a header template :) can we merge this PR and will add it with my current PR ?

@skendrot

skendrot commented Jul 9, 2017

Copy link
Copy Markdown
Contributor

Header and HeaderTemplate go have in hand. Just like Content and ContentTemplate

@nmetulev

Copy link
Copy Markdown
Contributor Author

@IbraheemOsama, do you mind pushing the ContentTemplate to this PR, you should just be able to push to this branch?

@IbraheemOsama

Copy link
Copy Markdown
Member

@nmetulev sure, no problem at all, will push ContentTemplate to this branch.

@IbraheemOsama

Copy link
Copy Markdown
Member

@skendrot @nmetulev ping :)

/// </summary>
public static readonly DependencyProperty HeaderTemplateProperty = DependencyProperty.Register(nameof(HeaderTemplate), typeof(DataTemplate), typeof(MenuItem), new PropertyMetadata(null));

/// <summary>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think this should be updated

set { SetValue(HeaderProperty, value); }
}

/// <summary>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Should point to HeaderTemplate (not Header)

@nmetulev

Copy link
Copy Markdown
Contributor Author

@IbraheemOsama, you are reading my mind cause you fixed it at the same time as I commented.

Looks good to me

@nmetulev
nmetulev merged commit 0318d18 into dev Jul 12, 2017
@nmetulev
nmetulev deleted the menuitemheader branch July 12, 2017 21:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants