Skip to content

Conversation

@bijington
Copy link
Contributor

@bijington bijington commented Jun 17, 2025

Description of Change

Linked Issues

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

@TheCodeTraveler
Copy link
Collaborator

TheCodeTraveler commented Jun 17, 2025

Thanks Shaun!

I advocate against implementing this because there are already ways for developers to pass in values to their popup via Dependency Injection, DataBinding, Messaging Services, etc.

Yes, I know this feature existed in Popup v1, but to be honest it is a bit of anti-pattern implementing a messaging service into Popup and we should have never supported it. I am not aware of any developer who is blocked without this feature.

If a developer can provide a sample app demonstrating how they are blocked without this feature, I'm happy to readdress this feature.

@TheCodeTraveler TheCodeTraveler added the needs discussion Discuss it on the next Monthly standup label Jun 17, 2025
@bijington
Copy link
Contributor Author

This scenario only introduces the ability to pass data in at the point of creation/navigating so I don't think DataBinding or Messaging Services are suitable alternatives.

As for developers being blocked there are some over here: #2696 (comment)

@VladislavAntonyuk
Copy link
Collaborator

@bijington could you please explain how they will use the parameters? PopupPage is internal and I don't know how it replaces onPresenting.

I agree with @TheCodeTraveler. At least they can subscribe to Opened event and set parameters they need.

@bijington
Copy link
Contributor Author

bijington commented Jun 18, 2025

@bijington could you please explain how they will use the parameters? PopupPage is internal and I don't know how it replaces onPresenting.

I agree with @TheCodeTraveler. At least they can subscribe to Opened event and set parameters they need.

Happy to... this doesn't introduce any new concepts it just makes the use of Popups in non-Shell based apps symmetrical with Shell based apps. The parameters would work as per our documentation https://learn.microsoft.com/en-us/dotnet/communitytoolkit/maui/views/popup-service#passing-data-to-a-popup-view-model only for the methods that accept INavigation and not only Shell.

My intention was never to replace onPresenting, I would have left that in. IQueryAttributable was a compromise but the initial implementation only supported Shell based apps

@VladislavAntonyuk
Copy link
Collaborator

Yes, but you cast popupPage to IQueryAttributable. Maybe you should do it with ViewModel or View?

@bijington
Copy link
Contributor Author

Yes, but you cast popupPage to IQueryAttributable. Maybe you should do it with ViewModel or View?

Oh I see your point. PopupPage is a wrapper for the content so it delegates down to the content view/view model. I've removed the explicit implementation of IQueryAttributable so we don't need to cast

@dotnet-policy-service dotnet-policy-service bot 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 Jul 19, 2025
@VladislavAntonyuk VladislavAntonyuk removed the needs discussion Discuss it on the next Monthly standup label Aug 7, 2025
@bijington bijington closed this Aug 28, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Aug 30, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

help wanted This proposal has been approved and is ready to be implemented stale The author has not responded in over 30 days

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants