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

Add popup sizing page to cover more test scenarios #1770

Merged
merged 7 commits into from
Mar 27, 2024

Conversation

bijington
Copy link
Contributor

Description of Change

This PR doesn't aim to fix anything, its purpose is to provide a more comprehensive set of Popup based samples to make verification of change/fixes easier.

The current approach relies on a bit of magic:

void OnShowPopup(Page page)
{
    var popup = new Popup();

    if (SelectedContainer?.ControlTemplate.LoadTemplate() is not View container)
    {
        return;
    }

    container.GetType().GetProperty(nameof(IPaddingElement.Padding))?.SetValue(container, new Thickness(Padding));

    const string longText = "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.";

    container.GetType().GetProperty(nameof(IContentView.Content))?.SetValue(container, GetContentLabel(longText));

    if (container is Layout layout)
    {
        layout.Children.Add(GetContentLabel(longText));
    }
    else if (container is ItemsView itemsView)
    {
        itemsView.ItemsSource = Enumerable.Repeat(longText, 10);
        itemsView.ItemTemplate = new DataTemplate(() => GetContentLabel(longText));
    }

    popup.Content = container;

    page.ShowPopup(popup);
}

And it is driven from a list of Containers:

public IList<ContainerViewModel> Containers { get; } =
[
    new ("HorizontalStackLayout", new ControlTemplate(() => new HorizontalStackLayout())),
    new ("VerticalStackLayout", new ControlTemplate(() => new VerticalStackLayout())),
    new ("Border", new ControlTemplate(() => new Border())),
    new ("Grid", new ControlTemplate(() => new Grid())),
    new ("CollectionView", new ControlTemplate(() => new CollectionView()))
];

I couldn't decide whether this saved on effort over a single clearly defined scenario e.g.

public IList<ContainerViewModel> Scenarios { get; } =
[
    new ("HorizontalStackLayout with Label", new ControlTemplate(() => new HorizontalStackLayout { Children = [new Label()] }))
];

The part I really like is the effort we save by not having to create a popup xaml/c# for each scenario.

Thoughts?

Linked Issues

n/a

PR Checklist

  • Has a linked Issue, and the Issue has been approved(bug) or Championed (feature/proposal)
  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard
  • Documentation created or updated: https://github.com/MicrosoftDocs/CommunityToolkit/pulls

Additional information

@VladislavAntonyuk
Copy link
Collaborator

The only suggestion to move all Popup to another section/group. We already have a page with many buttons to show different popups, maybe it make sense to move all samples related to popup to the page

@bijington
Copy link
Contributor Author

The only suggestion to move all Popup to another section/group. We already have a page with many buttons to show different popups, maybe it make sense to move all samples related to popup to the page

Oh yeah that's a great idea

Copy link
Collaborator

@brminnick brminnick left a comment

Choose a reason for hiding this comment

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

Great idea Shaun!!

It looks like the HorizontalStackLayout and VerticalStackLayout has an issue on the new PopupSizingIssuesPage, but since that bug isn't caused by this PR I'm good to merge this 👍

@cat0363 - as our resident Popup expert, any chance you're familiar with these Popup sizing/layout bugs?

HorizontalStackLayout VerticalStackLayout Border
image image image

@brminnick brminnick enabled auto-merge (squash) March 26, 2024 00:32
@brminnick brminnick merged commit 85f2315 into main Mar 27, 2024
8 checks passed
@brminnick brminnick deleted the feature/sl-add-popup-sizing-page branch March 27, 2024 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants