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

Staggered panel #1713

Merged
merged 17 commits into from
Jan 22, 2018
Merged

Conversation

skendrot
Copy link
Contributor

Issue: #21

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] 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:

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?

New panel control for displaying content in a staggered approach.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@JohnnyWestlake
Copy link
Contributor

JohnnyWestlake commented Jan 12, 2018

Thoughts on making this orientation aware with an Orientation property, so it can optionally stack in rows instead of columns? Doesn't seem like it'd take much to do it

@skendrot
Copy link
Contributor Author

I did think about that, but decided against because the use-cases I came up with order mattered. Meaning that items would need to be in a particular row, rather than a random row. Do you have a good use-case for changing orientation?

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.

Looking good other than the resize bug

private int GetColumnIndex(double[] columnHeights)
{
int columnIndex = 0;
double height = columnHeights[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

Got an exeption here when the width of the control was less than the first column.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! fixed.

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.

Works beautifully 💯

@IbraheemOsama IbraheemOsama self-requested a review January 19, 2018 23:17
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.

Unfortunately couldn't find something major :(


private void OnHorizontalAlignmentChanged(DependencyObject sender, DependencyProperty dp)
{
InvalidateMeasure();
Copy link
Member

Choose a reason for hiding this comment

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

I think it is better to InvalidateArrange to restart the whole process and InvalidateArrange will call InvalidateMeasure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calling InvalidateArrange will only call into ArrangeOverride and not MeasureOverride. When these change we need to remeasure for layout. We probably could only call InvalideArrange if the HorizontalAlignment was not going to/from Stretch

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, this would force us to keep the previous state as we don't know what the old value was when using RegisterPropertyChangedCallback

{
if (d is StaggeredPanel panel)
{
panel.InvalidateMeasure();
Copy link
Member

Choose a reason for hiding this comment

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

Same thing for invalidateArrange

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calling InvalidateArrange will only call into ArrangeOverride and not MeasureOverride. When this value changes we need to remeasure for layout.


private static void OnDesiredColumnWidthChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
{
if (d is StaggeredPanel panel)
Copy link
Member

Choose a reason for hiding this comment

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

you can cast directly as this property will not be used anywhere else

@skendrot
Copy link
Contributor Author

@nmetulev can you re-review? I added the Padding property to the panel

@IbraheemOsama IbraheemOsama merged commit 0084504 into CommunityToolkit:master Jan 22, 2018
@windowstoolkitbot
Copy link

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

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.

5 participants