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(Expander): improve Expander #1478

Merged
merged 7 commits into from
Oct 31, 2017
Merged

feat(Expander): improve Expander #1478

merged 7 commits into from
Oct 31, 2017

Conversation

Odonno
Copy link
Contributor

@Odonno Odonno commented Sep 5, 2017

Improvements on Expander control.

  • Content overlay (display mode for overlay)
  • Animations on Content and Header

Linked to #966 and #924

@dotMorten
Copy link
Contributor

dotMorten commented Sep 6, 2017

Could you provide a description? It would make it a little easier evaluating the PR if we know what it does :)
Is there just a missing link to an issue?

@Odonno
Copy link
Contributor Author

Odonno commented Sep 6, 2017

@dotMorten I'll add more info but first I need to make the code work. I asked @IbraheemOsama to see what is wrong with my code.

To make it clear, I have an error when I have both a ContentPresenter and a ContentControl visible in the template of the control.

</VisualState>
<VisualState x:Name="OverlayHidden">
<VisualState.Setters>
<Setter Target="PART_MainContent" Value="Visible" />
Copy link
Member

Choose a reason for hiding this comment

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

replace the setters with this. you were missing the Visibility property :)
<Setter Target="PART_MainContent.Visibility" Value="Visible" /> <Setter Target="PART_ContentOverlay.Visibility" Value="Visible" />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OMG. That makes sense. Thanks, I will fix that.

/// <summary>
/// Gets or sets a value indicating whether the ContentOverlay of the control.
/// </summary>
public object ContentOverlay
Copy link
Member

Choose a reason for hiding this comment

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

This should be
public UIElement ContentOverlay
{
get { return (UIElement)GetValue(ContentOverlayProperty); }
set { SetValue(ContentOverlayProperty, value); }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worry. I tried with object to see if it worked better.

Copy link
Member

@IbraheemOsama IbraheemOsama left a comment

Choose a reason for hiding this comment

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

Just a single fix and the code will work just fine :)

@Odonno
Copy link
Contributor Author

Odonno commented Sep 6, 2017

Thanks for your help @IbraheemOsama

Still need to make some animations and update the docs.

@nmetulev
Copy link
Contributor

Just FYI, there seems to be few conflicts with the master branch

@Odonno
Copy link
Contributor Author

Odonno commented Sep 29, 2017

@nmetulev Yes. Need to find time to make animations.

@Odonno
Copy link
Contributor Author

Odonno commented Sep 30, 2017

@nmetulev I am a little stuck with animation of Content or Overlay elements.

I want to add a Storyboard on the intersection of two VisualStates. Example: When Expanded & RightDirection, I should play a specific animation based on the visiblity mode and the direction.

@JohnnyWestlake
Copy link
Contributor

@Odonno It wouldn't be untoward to have different states for each expanded direction (i.e. ExpandedRight, ExpandedLeft etc) - a few controls in the platform have similarly overlapping states which does create more states than one would first think necessary, but it is what it is.

@Odonno
Copy link
Contributor Author

Odonno commented Oct 2, 2017

@JohnnyWestlake Well, that means 16 VisualStates. It's a lot but if it's the only way, let's go for it.

@JohnnyWestlake
Copy link
Contributor

@Odonno well, it'd be the most "standard" way, but spinning ideas you could also have all the transition storyboards you want as resources, and dynamically change the actual VisualTransition storyboard just before state changes ( and let developers know in the documentation about this ) (keeping in mind Storyboards can also have child storyboards inside them). It'd just be an non-standard way of doing things.

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.

Looks good to me

@azchohfi azchohfi merged commit cdcc3cf into CommunityToolkit:master Oct 31, 2017
@Odonno Odonno deleted the expanderImprovements branch November 1, 2017 10:00
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.

8 participants