Fix AdaptiveGridView OneRowMode height

[skendrot]

Issue: #1613

PR Type

What kind of change does this PR introduce?

[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?

Height of the items does not match the requested height

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?

Height of items should now match the requested height

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

[michael-hawker]

Original Example:

Looks like the behavior has changed with this change. The panel will now take up all the vertical space in one row mode (in the image above the scrollbar is right beneath the images). I have to add VerticalAlignment="Center" to the AdaptiveGridView to make it behave like it did previously.

It's also still clipping by 4 pixels (better than the 14 before though).

You can check by changing the border thickness in the template to 5 and Red:

You can see the collapsed scrollbar at the bottom-left without the added vertical alignment.

[michael-hawker]

Could this be related to the collapsible scrollbars in the platform behaving differently between vertical and horizontal?

In vertical we can see the overlap clipping when the scrollbar is expanded (but it's translucent):

But the display isn't disrupted when it's collapsed:

[michael-hawker]

As per above, forgot to make it an actual review, as it was behavioral. Looks like there's a bit more going on here than the simple height change.

[nmetulev]

ping @skendrot

[skendrot]

Looks like the border being cut off is from the GridViewItem style being applied

style.Setters.Add(new Setter(MarginProperty, new Thickness(0, 0, 0, 4)));

Setting the Margin to 0 shows the entire element.

[skendrot]

I have a solution that allows for the same ScrollBar behavior as before, but allows to see the entire item. It involves an internal ValueConverter for the MaxHeight binding.

Thoughts on this direction?

        public object Convert(object value, Type targetType, object parameter, string language)
        {
            if (value != null)
            {
                var gridView = parameter as AdaptiveGridView;
                if (gridView == null)
                {
                    return value;
                }

                var padding = gridView.Padding;
                int.TryParse(value.ToString(), out int height);

                height = height + (int)padding.Top + (int)padding.Bottom;
                var setter = gridView.ItemContainerStyle.Setters.OfType<Setter>().FirstOrDefault(s => s.Property == FrameworkElement.MarginProperty);
                if (setter != null)
                {
                    var margin = (Thickness)setter.Value;
                    height = height + (int)margin.Top + (int)margin.Bottom;
                }

                return height;
            }

            return value;
        }

[nmetulev]

Looks like you need to update the headers on the new file

[nmetulev]

ping @michael-hawker for his review

[skendrot]

ping @michael-hawker

[michael-hawker]

Sorry for the delay on this, been swamped the last bit.

Minor thought on access, but tested it out and looks great!

[michael-hawker]

If someone wants to adapt the adaptive grid view, won't they need access to this?

Should we instead make it public but in a sub-namespace like Microsoft.Toolkit.Uwp.UI.Controls.AdaptiveGridView?

[skendrot]

I don't see any reason it needs to be public. It's used within a private method. Can always open it at a later date

[windowstoolkitbot]

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