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

PopupService #1165

Merged
merged 28 commits into from
Nov 1, 2023
Merged

PopupService #1165

merged 28 commits into from
Nov 1, 2023

Conversation

bijington
Copy link
Contributor

@bijington bijington commented Apr 28, 2023

Description of Change

Linked Issues

PR Checklist

Additional information

@bijington bijington mentioned this pull request Apr 30, 2023
8 tasks
src/CommunityToolkit.Maui/PopupService.cs Outdated Show resolved Hide resolved
src/CommunityToolkit.Maui/PopupService.cs Outdated Show resolved Hide resolved
@ghost ghost added stale The author has not responded in over 30 days help wanted This proposal has been approved and is ready to be implemented labels Jun 10, 2023
@MDale-RAC
Copy link

Any updates on this?

@bijington
Copy link
Contributor Author

Any updates on this?

No I haven't been able to determine what the best option is for IArgumentsReceiver - whether we want a different name or better abstraction. Any input is welcome :)

@bijington
Copy link
Contributor Author

As an alternative to requiring an interface when presenting the popup for the view model to receive data we could try something like this:

public Task<object?> ShowPopupAsync<TViewModel>(Action<TViewModel> onPresenting) where TViewModel : INotifyPropertyChanged
{
    ArgumentNullException.ThrowIfNull(onPresenting);

    var popup = GetPopup(typeof(TViewModel));

    var viewModel = GetViewModel<TViewModel>();

    popup.BindingContext = viewModel;

    onPresenting.Invoke(viewModel);

    return CurrentPage.ShowPopupAsync(popup);
}

The onPresenting action would be invoked once the view model is created and assigned to the popup BindingContext, just before the popup is presented.

It could then be used as follows:

public class MyViewModel : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler? PropertyChanged;

    public void LoadData(int id)
    {
    }
}

void Show()
{
    ShowPopupAsync<MyViewModel>(onPresenting: viewModel => viewModel.LoadData(123));
}

What do people think? Is it clean or a bit yucky?

@VladislavAntonyuk
Copy link
Collaborator

As an alternative to requiring an interface when presenting the popup for the view model to receive data we could try something like this:

public Task<object?> ShowPopupAsync<TViewModel>(Action<TViewModel> onPresenting) where TViewModel : INotifyPropertyChanged
{
    ArgumentNullException.ThrowIfNull(onPresenting);

    var popup = GetPopup(typeof(TViewModel));

    var viewModel = GetViewModel<TViewModel>();

    popup.BindingContext = viewModel;

    onPresenting.Invoke(viewModel);

    return CurrentPage.ShowPopupAsync(popup);
}

The onPresenting action would be invoked once the view model is created and assigned to the popup BindingContext, just before the popup is presented.

It could then be used as follows:

public class MyViewModel : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler? PropertyChanged;

    public void LoadData(int id)
    {
    }
}

void Show()
{
    ShowPopupAsync<MyViewModel>(onPresenting: viewModel => viewModel.LoadData(123));
}

What do people think? Is it clean or a bit yucky?

I like it, but make it Func<ViewModel, Task>

src/CommunityToolkit.Maui/PopupService.cs Outdated Show resolved Hide resolved
src/CommunityToolkit.Maui/PopupService.cs Outdated Show resolved Hide resolved
@bijington bijington marked this pull request as ready for review August 13, 2023 22:18
@ghost ghost added stale The author has not responded in over 30 days help wanted This proposal has been approved and is ready to be implemented labels Oct 18, 2023
@bijington bijington changed the title Initial implementation of a possible PopupService Implementation of a possible PopupService Oct 26, 2023
@bijington bijington requested review from brminnick and pictos October 26, 2023 16:36
src/CommunityToolkit.Maui/PopupService.cs Outdated Show resolved Hide resolved
@VladislavAntonyuk VladislavAntonyuk removed the stale The author has not responded in over 30 days label Oct 31, 2023
@brminnick brminnick changed the title Implementation of a possible PopupService PopupService Oct 31, 2023
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.

Thanks Shaun!! I'm excited to see PopupService finally become a reality!!

I'm going to merge this right away so that we can squeeze it into this week's release.

Could you prioritize getting the PopupService docs done before Monday so that PopupService doesn't go too long without proper documentation?

FYI - I made a small tweak, removing PopupService's functionality that automatically assigned BindingContexts. I renamed the method to ValidateBindingContext (below) to ensure that the user has properly assigned the Popup View's BindingContext. However, I think it's out-of-scope for the PopupService to automatically assign it for them.

For example, .NET MAUI doesn't automatically assign a ContentPage's BindingContext when a user passes it into IServiceCollection.

I'm happy to circle back to this discussion in the future if we get requests from the community. But I agree that PopupService should be opinionated, and it should require the correct BindingContext. And us throwing an InvalidOperationException is enough to inform the dev of the requirement, in lieu of doing it for them under the hood (which they may not expect/want).

static void ValidateBindingContext<TViewModel>(Popup popup, out TViewModel bindingContext)
{
 if (popup.BindingContext is not TViewModel viewModel)
 {
 	throw new InvalidOperationException($"Unexpected type has been assigned to the BindingContext of {popup.GetType().FullName}. Expected type {typeof(TViewModel).FullName} but was {popup.BindingContext?.GetType().FullName?? "null"}");
 }

 bindingContext = viewModel;
}

@brminnick brminnick enabled auto-merge (squash) November 1, 2023 22:26
@brminnick brminnick removed the help wanted This proposal has been approved and is ready to be implemented label Nov 1, 2023
@brminnick brminnick merged commit 2ec4be1 into main Nov 1, 2023
7 checks passed
@brminnick brminnick deleted the feature/sl/981-add-popupservice branch November 1, 2023 22:44
@github-actions github-actions bot locked and limited conversation to collaborators Nov 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Proposal] PopupService
8 participants