-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Headered controls #1599
Headered controls #1599
Conversation
…ect Border properties
xmlns:controls="using:Microsoft.Toolkit.Uwp.UI.Controls"> | ||
<Style TargetType="controls:HeaderedContentControl"> | ||
<Setter Property="HorizontalContentAlignment" Value="Left"/> | ||
<Setter Property="VerticalContentAlignment" Value="Top"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, why not stretch for these two values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follows the style of the ContentControl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be in the documentation. As will information about why the Border* properties are not respected (b/c the ContentControl and ItemsControl do not)
{ | ||
} | ||
|
||
private static void OnHeaderChanged(DependencyObject d, DependencyPropertyChangedEventArgs e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't look like this is referenced anywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
/// </summary> | ||
/// <param name="oldValue">The old value of the <see cref="Header"/> property.</param> | ||
/// <param name="newValue">The new value of the <see cref="Header"/> property.</param> | ||
protected virtual void OnHeaderChanged(object oldValue, object newValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following API from WPF
{ | ||
} | ||
|
||
private static void OnHeaderChanged(DependencyObject d, DependencyPropertyChangedEventArgs e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following API from WPF
Looks good to me (other than the missing docs) :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pretty good addition to the Toolkit. I left some comments but it seems good to me as is.
{ | ||
"Name": "HeaderedContentControl", | ||
"Type": "HeaderedContentControlPage", | ||
"About": "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing description?
{ | ||
"Name": "HeaderedItemsControl", | ||
"Type": "HeaderedItemsControlPage", | ||
"About": "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing description?
The `Header` property can be set to a string, or any xaml elements. If binding the `Header` to an object that is not a string, use the `HeaderTemplate` to control how the content is rendered. | ||
```xaml | ||
<controls:HeaderedControlControl Header="This is the header!"/> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we split into 2 code blocks?
There is a conflict with the master branch. |
# Conflicts: # Microsoft.Toolkit.Uwp.SampleApp/Microsoft.Toolkit.Uwp.SampleApp.csproj
It didn't yesterday! 😝 |
This PR is linked to unclosed issues. Please check if one of these issues should be closed: #995 |
Related: #3710 (Missing metadata for the DesignTime in VS IDE) |
Issue: #995
PR Type
What kind of change does this PR introduce?
Add HeaderedContentControl and HeaderedItemsControl
What is the current behavior?
PR Checklist
Please check if your PR fulfills the following requirements:
What is the new behavior?
Does this PR introduce a breaking change?
Other information