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

BladeItem fix for #1218 #1609

Merged
merged 9 commits into from
Nov 11, 2017
Merged

Conversation

skendrot
Copy link
Contributor

Issue: #1218

PR Type

What kind of change does this PR introduce?
Add default foreground properties to the default style

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build or CI related changes
[ ] Documentation content changes
[ ] Sample app changes
[ ] Other... Please describe:

What is the current behavior?

Weird bug by the style not defining the foregrounds

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Docs have been added / updated (for bug fixes / features)
  • Sample in sample app has been added / updated (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)

What is the new behavior?

Foregrounds are now part of the dele

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Marked the Title property as obsolete in favor of the Header property from the Expander

Copy link
Contributor

@nmetulev nmetulev left a comment

Choose a reason for hiding this comment

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

This is great. Should the sample be updated to use the Header property?

@skendrot
Copy link
Contributor Author

Yup!

Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Just want to make sure we maintain layout behavior between the old and new?

@@ -32,9 +32,6 @@ public BladeItemMetadata()
b.AddCustomAttributes(nameof(BladeItem.TitleBarVisibility),
new CategoryAttribute(Properties.Resources.CategoryCommon)
);
b.AddCustomAttributes(nameof(BladeItem.Title),
Copy link
Member

Choose a reason for hiding this comment

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

Does Header not need to take its place here?

Or is that taken care of by the Expander inheritance in this situation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should be part of the Expander

@@ -132,12 +134,10 @@
<ColumnDefinition Width="Auto" />
</Grid.ColumnDefinitions>

<TextBlock x:Name="TitleBar"
Margin="4,0,0,0"
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't look like there's a default template defined which maintains this padding and alignment for the text within the presenter?

We should make sure the visual behavior is consistent still with this type of change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out, fixed!

Copy link
Member

Choose a reason for hiding this comment

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

That's what CRs are for :).

Don't forget the HorizontalAlignment Property though too, called it out above.

<Setter Property="HeaderTemplate">
<Setter.Value>
<DataTemplate>
<TextBlock Text="{Binding}" Margin="4,0,0,0" VerticalAlignment="Center"/>
Copy link
Member

Choose a reason for hiding this comment

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

Don't forget the HorizontalAlignment either, default is Stretch not Left, as it was before. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But TextBlock aligns to the left, so it's handled

Copy link
Member

Choose a reason for hiding this comment

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

image

Well, the HorizontalTextAlignment is Left but the HorizontalAlignment is still Stretch. Practically though, it won't make a difference unless someone was using the VisualTree to fiddle with stuff like Width; which for this case now, is to just use their own Header.

@nmetulev nmetulev merged commit eed1599 into CommunityToolkit:master Nov 11, 2017
@windowstoolkitbot
Copy link

This PR is linked to unclosed issues. Please check if one of these issues should be closed: #1218

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

Successfully merging this pull request may close these issues.

4 participants