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.

Sign up for GitHub

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

Jump to bottom

[Proposal] Enhance PopupService to provide a set of Close methods #1530

Closed
8 tasks
bijington opened this issue Nov 15, 2023 · 6 comments · Fixed by #1688
Closed
8 tasks

[Proposal] Enhance PopupService to provide a set of Close methods #1530

bijington opened this issue Nov 15, 2023 · 6 comments · Fixed by #1688
Labels
area/views Issue/Discussion/PR that has to do with Views new proposal A fully fleshed out proposal describing a new feature in syntactic and semantic detail

Comments

@bijington
Copy link
Contributor

Feature name

Enhance PopupService to provide a set of Close methods

Link to discussion

#1529

Progress tracker

  • Android Implementation
  • iOS Implementation
  • MacCatalyst Implementation
  • Windows Implementation
  • Tizen Implementation
  • Unit Tests
  • Samples
  • Documentation

Summary

We now have the ability to Show popups from view models without any view specific knowledge. Now let's take this functionality further and give developers the ability to Close them too.

Motivation

To make all of our lives easier. Provides symmetry in the PopupService by enabling the ability to close as well as show.

Detailed Design

IPopupService

public interface IPopupService
{
    // Closes the currently displayed popup. Optionally returning a result.
    void Close(object? result = null);

    // Closes the currently displayed popup.  Optionally returning a result.
    void CloseAsync(object? result = null, CancellationToken cancellationToken = default);
}

PopupService

public class PopupService
{
    // Might need to be Stack<Popup> or something else in order to track multiple popups being displayed.
    Popup? currentPopup; // Assigned in each of the ShowPopup calls

    // Closes the currently displayed popup. Optionally returning a result.
    public void Close(object? result = null)
    {
        if (currentPopup is null)
        {
            return; // Should we throw?
        }

        currentPopup.Close(result);
    }

    // Closes the currently displayed popup.  Optionally returning a result.
    public void CloseAsync(object? result = null, CancellationToken cancellationToken = default)
    {
        if (currentPopup is null)
        {
            return; // Should we throw?
        }

        return currentPopup.CloseAsync(result, cancellationToken);
    }
}

Usage Syntax

// Closing a popup
popupService.ClosePopup();

// Closing a popup with a result
popupService.ClosePopup(result: true);

Drawbacks

Introduces complexity around maintaining the knowledge of displayed popups.

Alternatives

No response

Unresolved Questions

Do we want to allow for closing based on the view model type? e.g. PopupService.Close<TViewModel>();
@bijington bijington added new proposal A fully fleshed out proposal describing a new feature in syntactic and semantic detail labels Nov 15, 2023
@brminnick
Copy link
Collaborator

brminnick commented Nov 15, 2023 •
edited
Loading

Since multiple Popups can be displayed concurrently, it'd be best to have the developer specify which popup they'd like to close.

void ClosePopup<TPopup>(TPopup poup) where TPopup : IPopup;

void ClosePopup<TPopup>(TPopup poup, object? result = null) where TPopup : IPopup;

void ClosePopup<TPopup>(object? result = null) where TPopup : IPopup;

Task ClosePopupAsync<TPopup>(TPopup poup) where TPopup : IPopup;

Task ClosePopupAsync<TPopup>(TPopup poup, object? result = null) where TPopup : IPopup;

Task ClosePopupAsync<TPopup>(object? result = null) where TPopup : IPopup;

Maybe not for this Proposal, but I could also see us adding another API in the future that closes all open popups:

void CloseAllPopups();
Task CloseAllPopupsAsync();

@bijington
Copy link
Contributor Author

@brminnick you make a good point re: multiple popups. I like the API surface you have proposed although I think we want to avoid the reliance on a developer providing the Popup type as this is view specific information. We should be able to allow developers to provide the view model like we do with Show and handle the mappings internally to find the associated Popup.

What do you think?

@brminnick
Copy link
Collaborator

I think we want to avoid the reliance on a developer providing the Popup type as this is view specific information

Yup - that's totally cool with me, because ClosePopup<TPopup>() and ClosePopupAsync<TPopup>() still accomplish the same objective 💯

@bijington bijington added the needs discussion Discuss it on the next Monthly standup label Nov 29, 2023
@bijington
Copy link
Contributor Author

I've been thinking on this a little more. While developers can show multiple popups they are effectively stacked on top of each other right? As a user is prevented from interacting with a popup below another popup.

I need to test some of this further but if it is the case then we could store the displayed popups inside a Stack and pop them off each time we close.

@vhugogarcia vhugogarcia added the area/views Issue/Discussion/PR that has to do with Views label Dec 7, 2023
@bobbydharrell
Copy link

bobbydharrell commented Jan 17, 2024 •
edited
Loading

I would like to add another request to this, please. Add a way to close the Popups via ViewModel such as ClosePopup<TViewModel>() and ClosePopupAsync<TViewMOdel>()

Unless I misunderstood the Proposed fixes

@bobbydharrell
Copy link

bobbydharrell commented Jan 17, 2024 •
edited
Loading

@bijington your proposed stack along with closing via ViewModel is exactly what we need, Thank you .. That would solve all of my concerns .. as long as we can do a close all on the stack

@bijington bijington removed the needs discussion Discuss it on the next Monthly standup label Feb 1, 2024
@bijington bijington mentioned this issue Feb 11, 2024
Merged
6 tasks
@github-project-automation github-project-automation bot moved this to Proposal Submitted in New Feature Proposals Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/views Issue/Discussion/PR that has to do with Views new proposal A fully fleshed out proposal describing a new feature in syntactic and semantic detail
Projects
New Feature Proposals
Status: Proposal Submitted
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Implement the ability to Close a popup CommunityToolkit/Maui
4 participants
@vhugogarcia @bobbydharrell @brminnick @bijington