Skip to content

Add Popup Control#290

Merged
TheCodeTraveler merged 157 commits intomainfrom
pj/popup-to-handler
Mar 14, 2022
Merged

Add Popup Control#290
TheCodeTraveler merged 157 commits intomainfrom
pj/popup-to-handler

Conversation

@pictos
Copy link
Copy Markdown
Member

@pictos pictos commented Feb 10, 2022

Description of Change

This PR is the initial work of the Popup handler and the basement for other Handlers in MCT.

Somethings to be aware of:

  • WinUI is not so great, we can open issues to fix it and get community help on that;
  • Platform-specific APIs are not contemplated in this PR;
  • Not have automated tests, I need to work in the Unit device test suite;

Linked Issues

PR Checklist

Additional information

pictos added 30 commits January 23, 2022 11:32
…pj/popup-to-handler

# Conflicts:
#	samples/CommunityToolkit.Maui.Sample/MauiProgram.cs
#	samples/CommunityToolkit.Maui.Sample/Models/SectionModel.cs
#	samples/CommunityToolkit.Maui.Sample/ViewModels/MainGalleryViewModel.cs
@pictos
Copy link
Copy Markdown
Member Author

pictos commented Mar 10, 2022

@brminnick there's something else that you want before merge this?

@TheCodeTraveler
Copy link
Copy Markdown
Collaborator

TheCodeTraveler commented Mar 11, 2022

Yes! I wanted to circle back to our discussion about the naming for LightDismiss now that we're getting close to merging Popup!

Concern

As someone who primarily works on iOS + Android (and rarely works on Windows), I was unfamiliar with Windows' Light Dismiss nomenclature and I personally had to google LightDismiss in order to understand this feature.

My concern is that other devs may have a similar experience and using Light Dismiss will also slow down their development velocity and decrease their discoverability of this feature.

That being said, I'm happy to be outvoted if most folks are familiar with the term Light Dismiss!

Research

(Copy/Pasting the info from the Code Review Conversation):

Light Dismiss seems to be a Windows term: https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.controls.primitives.popup.islightdismissenabled?view=winrt-22000

It does not seem to be an Android term: https://www.google.com/search?q=light+dismiss+Android

It does not seem to be an iOS term: https://www.google.com/search?q=light+dismiss+ios

Shaun's Recommendation

My vote is to move forward with @bijington naming recommendation:

  • OnClosed()
    • Triggers when the user taps a close button to dismiss a Popup
  • OnDismissed()
    • Triggers when the user taps outside of the Popup to dismiss it

Let's Vote!

Leave the following emoji reaction for your preference:
🚀 Keep LightDismiss and Dismiss
👀 Use Dismiss and Close
❤️ Use other naming convention (leave a comment with recommendation)

@VladislavAntonyuk
Copy link
Copy Markdown
Contributor

VladislavAntonyuk commented Mar 11, 2022

We can call it Hide instead of LightDismiss. Hide and Close
But another proposal:
Remove light dismiss at all.
Add boolean Backdrop property.
Backdrop=true prevents the popup closing on background click.
Add new event "hidePrevented" which is raised when user clicks on background.
Close/Dismiss just closes popup with specific event data.

Probably we can grab some ideas from https://docs.microsoft.com/en-us/dotnet/api/system.windows.controls.primitives.popup

@pictos
Copy link
Copy Markdown
Member Author

pictos commented Mar 11, 2022

@VladislavAntonyuk maybe we could call cancel instead of hide. Like hide gives an idea that the pop-up is not visible, but actually it was destroyed

@jfversluis
Copy link
Copy Markdown
Member

I'm a bit biased here as I already knew what light dismiss does, but I still voted for close/dismissed.

I feel cancelled wouldn't really cover it because there is still a way to get some kind of result out of the popup even when dismissed so it's not really cancelled? I don't know... Naming is hard :P

@VladislavAntonyuk
Copy link
Copy Markdown
Contributor

I agree with Gerald. Cancel for me is like an action. reverting some changes.
Does it matter for us how it was dismissed?
To keep it simple we can use only 1 method Dismiss with the reason parameter/result.

@bijington
Copy link
Copy Markdown
Contributor

That is a good point @VladislavAntonyuk. Sorry one bit I am missing, is there a reason we need to know the difference between a LightDismiss and a Dismiss? Other Popup controls seem to only offer a Closed command/method/event

Copy link
Copy Markdown
Collaborator

@TheCodeTraveler TheCodeTraveler left a comment

Choose a reason for hiding this comment

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

image

@TheCodeTraveler TheCodeTraveler enabled auto-merge (squash) March 14, 2022 20:48
@TheCodeTraveler TheCodeTraveler merged commit c1786d6 into main Mar 14, 2022
@delete-merged-branch delete-merged-branch bot deleted the pj/popup-to-handler branch March 14, 2022 21:20
@github-actions github-actions bot locked and limited conversation to collaborators Nov 24, 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] Popup

5 participants