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.

Sign up for GitHub

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

Expander Fixes #4394

Merged
merged 23 commits into from
Mar 26, 2021
Merged

Expander Fixes #4394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
1223fb1
Rename 'Expanding' to 'Expanded'
robloo Mar 2, 2021
363c732
Ensure Expanded/Collapsed fire after state is applied
robloo Mar 2, 2021
17c6d85
Remove MaxWidth set in the default style
robloo Mar 2, 2021
1f80b68
Make the expander properly size to content
robloo Mar 2, 2021
66bf68f
Remove Stretch content alignment (use Center default)
robloo Mar 2, 2021
0ad7bc5
Add Center VerticalAlignment
robloo Mar 2, 2021
ee209e1
Remove Min/MaxWidth from intermediate controls
robloo Mar 2, 2021
0c58a25
Update Expander test page
robloo Mar 2, 2021
25e8c59
Add fixed-height Expander example
robloo Mar 2, 2021
585b15c
Stretch header content
robloo Mar 2, 2021
21c73eb
Standardize resource names
robloo Mar 2, 2021
f2d4e00
The header VerticalContentAlignment must be center
robloo Mar 2, 2021
77554a7
Revert "Rename 'Expanding' to 'Expanded'"
robloo Mar 12, 2021
6a72d39
Revert "Ensure Expanded/Collapsed fire after state is applied"
robloo Mar 12, 2021
4739faf
Merge branch 'master' into robloo-expander-fixes
robloo Mar 12, 2021
210ca0c
Padding now applies to the Content area
robloo Mar 12, 2021
f081da4
Reorg and rename expander theme resources
robloo Mar 12, 2021
2a45b77
Work-around DP precedence issue
robloo Mar 13, 2021
9e9a810
Add resources to control header content alignment
robloo Mar 13, 2021
6543952
Fix Expander event ordering
robloo Mar 13, 2021
cf8bcef
Fix some Static not Theme resources
robloo Mar 13, 2021
ece7611
Remove some resource redirections
robloo Mar 16, 2021
53d69f4
Remove unnecessary margins
robloo Mar 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions dev/Expander/Expander.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,15 +103,27 @@ void Expander::RaiseCollapsedEvent(const winrt::Expander& container)

void Expander::OnIsExpandedPropertyChanged(const winrt::DependencyPropertyChangedEventArgs& /*args*/)
{
if (IsExpanded())
const auto isExpanded = IsExpanded();

if (isExpanded)
{
RaiseExpandingEvent(*this);
}
else
{
RaiseCollapsedEvent(*this);
// Available for a 'Collapsing' event
}

UpdateExpandState(true);

if (isExpanded)
{
// Available for an 'Expanded' event
}
else
{
RaiseCollapsedEvent(*this);
}
}

void Expander::OnExpandDirectionPropertyChanged(const winrt::DependencyPropertyChangedEventArgs& /*args*/)
Expand Down
44 changes: 21 additions & 23 deletions dev/Expander/Expander.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,15 @@
Without this override, the narrator user that focuses the expander on a touch screen will see that pressing "Tab"
doesn't work how they would expect.-->
<Setter Property="IsTabStop" Value="False"/>
<Setter Property="Background" Value="{ThemeResource ExpanderBackground}" />
<Setter Property="Background" Value="{ThemeResource ExpanderContentBackground}" />
robloo marked this conversation as resolved.
Show resolved Hide resolved
<contract7Present:Setter Property="BackgroundSizing" Value="InnerBorderEdge" />
<Setter Property="MinWidth" Value="{ThemeResource FlyoutThemeMinWidth}" />
<Setter Property="MaxWidth" Value="{ThemeResource FlyoutThemeMaxWidth}" />
<Setter Property="MinHeight" Value="{StaticResource ExpanderMinHeight}" />
<Setter Property="BorderThickness" Value="{ThemeResource ExpanderBorderThickness}" />
<Setter Property="BorderBrush" Value="{ThemeResource ExpanderBorderBrush}" />
<Setter Property="Padding" Value="{StaticResource ExpanderHeaderPadding}" />
<Setter Property="HorizontalAlignment" Value="Stretch" />
<Setter Property="HorizontalContentAlignment" Value="Left" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this HorizontalContentAlignment no longer needed? Looking at this change it looks like the template never actually used this before. Is this just deleting unused lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#4394 (comment)

  1. The default actually should be center per Expander Fixes #4394 (comment)
  2. I followed convention of the Button template
  3. When the default value of 'Center' is required, the property is simply omitted. This is better as all controls will fallback to the same default so if once in 1000 years the default was actually changed you wouldn't need to go and update all the templates (single source of truth). The rule of thumb: only change what you need to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tashatitova Did I misunderstand about the HorizontalContentAlignment? Were you only referring to VerticalContentAlignment in that comment referenced above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tashatitova Can you confirm about HorizontalContentAlignment? The more I think about it, I think this was my misunderstanding and you were only talking about VerticalContentAlignment. I would like to make this change and get this PR merged ASAP.

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 just re-read it and your last comment emphasizes vertical. I'm going to make the change and add back HorizontalContentAlignment. Horizontal seems like it should be Left so @StephenLPeters was right to point this out.

<Setter Property="VerticalContentAlignment" Value="Center" />
<Setter Property="BorderThickness" Value="{ThemeResource ExpanderContentDownBorderThickness}" />
<Setter Property="BorderBrush" Value="{ThemeResource ExpanderContentBorderBrush}" />
<Setter Property="Padding" Value="{StaticResource ExpanderContentPadding}" />
<Setter Property="HorizontalAlignment" Value="Left" />
<Setter Property="VerticalAlignment" Value="Center" />
<contract7Present:Setter Property="CornerRadius" Value="{ThemeResource ControlCornerRadius}" />
<Setter Property="Template">
robloo marked this conversation as resolved.
Show resolved Hide resolved
<Setter.Value>
Expand Down Expand Up @@ -110,7 +108,7 @@
<VisualState x:Name="Up">
<VisualState.Setters>
<Setter Target="ExpanderHeader.Style" Value="{StaticResource ExpanderHeaderUpStyle}" />
<Setter Target="ExpanderContent.BorderThickness" Value="{StaticResource ExpanderDropdownUpBorderThickness}" />
<Setter Target="ExpanderContent.BorderThickness" Value="{StaticResource ExpanderContentUpBorderThickness}" />
<contract7Present:Setter Target="ExpanderContent.CornerRadius" Value="{Binding CornerRadius, RelativeSource={RelativeSource TemplatedParent}, Converter={StaticResource TopCornerRadiusFilterConverter}}" />
<Setter Target="ExpanderHeader.(Grid.Row)" Value="1" />
<Setter Target="ExpanderContentClip.(Grid.Row)" Value="0" />
Expand All @@ -128,36 +126,36 @@
<ToggleButton
x:Name="ExpanderHeader"
AutomationProperties.AutomationId="ExpanderToggleButton"
Background="{TemplateBinding Background}"
Background="{ThemeResource ExpanderHeaderBackground}"
contract7Present:BackgroundSizing="{TemplateBinding BackgroundSizing}"
BorderBrush="{TemplateBinding BorderBrush}"
BorderThickness="{TemplateBinding BorderThickness}"
BorderBrush="{ThemeResource ExpanderHeaderBorderBrush}"
BorderThickness="{ThemeResource ExpanderHeaderBorderThickness}"
MinHeight="{TemplateBinding MinHeight}"
MinWidth="{TemplateBinding MinWidth}"
MaxWidth="{TemplateBinding MaxWidth}"
contract7Present:CornerRadius="{TemplateBinding CornerRadius}"
IsEnabled="{TemplateBinding IsEnabled}"
Padding="{TemplateBinding Padding}"
Padding="{StaticResource ExpanderHeaderPadding}"
Style="{StaticResource ExpanderHeaderDownStyle}"
HorizontalAlignment="Stretch"
HorizontalContentAlignment="{StaticResource ExpanderHeaderHorizontalContentAlignment}"
VerticalContentAlignment="{StaticResource ExpanderHeaderVerticalContentAlignment}"
Content="{TemplateBinding Header}"
ContentTemplate="{TemplateBinding HeaderTemplate}"
ContentTemplateSelector="{TemplateBinding HeaderTemplateSelector}"
HorizontalAlignment="{TemplateBinding HorizontalAlignment}"
IsChecked="{Binding Path=IsExpanded, Mode=TwoWay, RelativeSource={RelativeSource TemplatedParent}}" />
<!-- The clip is a composition clip applied in code -->
<Border x:Name="ExpanderContentClip" Grid.Row="1">
<Border
x:Name="ExpanderContent"
x:Name="ExpanderContent"
Visibility="Collapsed"
Background="{ThemeResource ExpanderDropDownBackground}"
Background="{TemplateBinding Background}"
contract7Present:BackgroundSizing="{TemplateBinding BackgroundSizing}"
contract7Present:CornerRadius="{Binding CornerRadius, RelativeSource={RelativeSource TemplatedParent}, Converter={StaticResource BottomCornerRadiusFilterConverter}}"
BorderBrush="{ThemeResource ExpanderDropDownBorderBrush}"
BorderThickness="{StaticResource ExpanderDropdownDownBorderThickness}"
BorderBrush="{TemplateBinding BorderBrush}"
BorderThickness="{StaticResource ExpanderContentDownBorderThickness}"
Copy link
Contributor Author

@robloo robloo Mar 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is not ideal.

  • BorderThickness="{StaticResource ExpanderContentDownBorderThickness}" should be
  • BorderThickness="{TemplateBinding BorderThickness}"

However, that would not work due to the BorderThickness of the control having local precedence (higher precedence than the visual state setter). The visual state is being used to switch between border thicknesses that have the top or bottom edge set to zero depending on the expand direction.

The correct solution to this is to add another filter converter for the border thickness similar to the CornerRadius. I expect push back so I didn't implement that.

What is done now is considered a hack but is usable for an initial release. Developers can still customize the thickness of the content area using ExpanderContentDownBorderThickness and ExpanderContentUpBorderThickness even while the BorderThickness of the control is ignored.

MinHeight="{TemplateBinding MinHeight}"
MinWidth="{TemplateBinding MinWidth}"
MaxWidth="{TemplateBinding MaxWidth}"
Padding="{StaticResource ExpanderContentPadding}">
HorizontalAlignment="Stretch"
tashatitova marked this conversation as resolved.
Show resolved Hide resolved
VerticalAlignment="Stretch"
Padding="{TemplateBinding Padding}">
<ContentPresenter
Content="{TemplateBinding Content}"
ContentTemplate="{TemplateBinding ContentTemplate}"
Expand Down
Loading