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

Fix layout when Margin of GridViewItem is not zero #1792

Merged
merged 5 commits into from
Feb 14, 2018

Conversation

skendrot
Copy link
Contributor

Issue: #1739

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[x] Feature

What is the current behavior?

If you define an ItemContainerStyle for the AdaptiveGridView and do not define a Margin, or if the Margin is different from what is defined by the AdaptiveGridView then items will not span the entire width of the control.

PR Checklist

Please check if your PR fulfills the following requirements:

What is the new behavior?

You can now define an ItemContainerStyle that does not define a Margin or a Margin different from the default and items will still layout properly

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

NOTE: This change has an added benefit that the AdaptiveGridView can now take advantage of an implicit style defined for a GridViewItem. This change removes the hard coded ItemContainerStyle which was preventing implicit styles to be used.

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.

Think this is good, I was able to customize the ItemContainerStyle, pretty cool.

I found a bug in the AdaptiveGridView, will file a separate issue, as not sure when it was introduced.

style.Setters.Add(new Setter(HorizontalContentAlignmentProperty, HorizontalAlignment.Stretch));
style.Setters.Add(new Setter(VerticalContentAlignmentProperty, VerticalAlignment.Stretch));
style.Setters.Add(new Setter(MarginProperty, new Thickness(0, 0, 0, 4)));
style.Setters.Add(new Setter(PaddingProperty, new Thickness(2, 0, 2, 0)));
Copy link
Member

Choose a reason for hiding this comment

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

The DefaultItemMargin below was 0,0,4,4, should it be updated to be 2,0,2,4 to match this or was that intentional?

If it's intentional that's fine, I think the buffer at the right/bottom is better than the split, but just wanted to make sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default margin for an item is 0,0,4,4. The default padding is 0. I think @nmetulev changed to this to help with knowing the width of the item

@skendrot skendrot merged commit 7f159ec into CommunityToolkit:master Feb 14, 2018
@windowstoolkitbot
Copy link

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

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