Skip to content

Conversation

@andrewleader
Copy link
Contributor

Issue: #

PR Type

What kind of change does this PR introduce?

Feature

What is the current behavior?

N/A

What is the new behavior?

Win32 desktop C# devs can install the notifications library to help facilitate sending toast notifications from non-UWP apps.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link
  • Sample in sample app has been added / updated (for bug fixes / features) (NOT APPLICABLE)
  • Tests for the changes have been added (for bug fixes / features) (if applicable) (NOT APPLICABLE)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

@michael-hawker
Copy link
Member

Think we had a separate thread somewhere; so wanted to capture it here, we want to make sure these work well with .NET Core 3.0 apps too.

@michael-hawker
Copy link
Member

We should also have a sample somewhere and a docs PR.

@andrewleader
Copy link
Contributor Author

@michael-hawker sample is here: https://github.com/WindowsNotifications/desktop-toasts

Docs are here: https://docs.microsoft.com/en-us/windows/uwp/design/shell/tiles-and-notifications/send-local-toast-desktop

Both haven't been updated yet since the NuGet package isn't published yet (but I'll update them once it is)

Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

@andrewleader merging this in, but QQ. Will this work with .NET Core or do we have to add targets for that?

@michael-hawker michael-hawker merged commit 541235e into master Aug 22, 2019
@delete-merged-branch delete-merged-branch bot deleted the nmetulev/desktopnotifications branch August 22, 2019 20:54
@andrewleader
Copy link
Contributor Author

andrewleader commented Aug 22, 2019

I think someone else (kbrons?) worked on ensuring .NET Core 3 support? I don't know personally

@michael-hawker
Copy link
Member

michael-hawker commented Aug 22, 2019

@nmetulev do you remember?

@andrewleader thanks for the quick response, let us know when you get the other doc repo going.

@michael-hawker michael-hawker mentioned this pull request Aug 23, 2019
27 tasks
@michael-hawker
Copy link
Member

@andrewleader talked to Nikola, we didn't look at .NET Core 3 support initially. Would you have time to look at that in the next couple of weeks? At least just to test and let us know if it works or not?

</ItemGroup>


<ItemGroup Condition="!('$(TargetFramework)'=='net461')">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm it indeed doesn't work on .NET Core 3.0 WPF apps, I think because of this line which excludes the DesktopNotificationManager code on everything except .NET Framework apps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The DesktopNotificationManager code also incorrectly gets included in ASP.NET Framework projects (when it should only be included in desktop WPF/console/client apps).

@michael-hawker is there any targeting we can do (on NuGet or in the compiled project) to only include the DesktopNotificationManager code for WPF (.NET Framework / .NET Core) and desktop apps, and NOT ASP.NET web projects?

Or do we simply need a separate NuGet package, Microsoft.Toolkit.Uwp.Notifications.Desktop? Separate package is okay with me, but it'd be neat if we could do it all in one and have it magically light up differently in all these different places

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants