-
Notifications
You must be signed in to change notification settings - Fork 401
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
SnackBar #5
SnackBar #5
Conversation
Hey there, please wait until we define what we will migrate to this repo and how. If I'm not wrong this repo isn't even with the XCT |
@pictos, @jfversluis, @brminnick please review. |
src/CommunityToolkit.Maui/Extensions/VisualElementExtension.shared.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui/Extensions/VisualElementExtension.shared.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui/Extensions/VisualElementExtension.shared.cs
Outdated
Show resolved
Hide resolved
...olkit.Maui/Views/Snackbar/Helpers/Apple/SnackbarViews/ActionMessageSnackBarView.ios.macos.cs
Outdated
Show resolved
Hide resolved
...nityToolkit.Maui/Views/Snackbar/Helpers/Apple/SnackbarViews/MessageSnackBarView.ios.macos.cs
Outdated
Show resolved
Hide resolved
...mmunityToolkit.Maui/Views/Snackbar/Helpers/Apple/SnackbarViews/BaseSnackBarView.ios.macos.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui/Views/Snackbar/Helpers/NativeRoundedStackView.ios.macos.cs
Outdated
Show resolved
Hide resolved
Hey @maxkoshevoi would you mind using the code review feature and post all comments at once instead of each one separately and killing my notifications? :) thanks! |
Hi @jfversluis, sorry about that! 😅 |
src/CommunityToolkit.Maui/Views/Snackbar/Options/ToastOptions.shared.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui/Views/Snackbar/Options/ToastOptions.shared.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui/Views/Snackbar/Options/SnackBarActionOptions.shared.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui/Views/Snackbar/Options/ToastOptions.shared.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui/Views/Snackbar/Options/MessageOptions.shared.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui/Views/Snackbar/Helpers/NativeSnackButton.ios.macos.cs
Outdated
Show resolved
Hide resolved
@brminnick Could you please update the task requirements according to the discussion (Use native Snackbar/toast for Android and popup for other platforms) |
Thanks for the reminder! I'll update the detailed design this weekend. Just curious - would you then be closing this PR and splitting it into two separate PRs; one for SnackBar and one for Toast? |
actually, snack bar and toast only differ with the action button. maybe I will update this PR with the snack bar implementation only and then add toast. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Vlad!
Just some quick semantic feedback since it looks like you're still working on it 🙌
src/CommunityToolkit.Maui/Extensions/VisualElementExtension.shared.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui/Extensions/VisualElementExtension.shared.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui/Extensions/VisualElementExtension.shared.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui/Extensions/VisualElementExtension.shared.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui/Views/Snackbar/Helpers/NativeRoundedStackView.ios.macos.cs
Outdated
Show resolved
Hide resolved
samples/CommunityToolkit.Maui.Sample/CommunityToolkit.Maui.Sample.csproj
Show resolved
Hide resolved
samples/CommunityToolkit.Maui.Sample/Pages/Views/SnackBarPage.xaml.cs
Outdated
Show resolved
Hide resolved
@brminnick what do you think if we split this feature on parts based on Platforms? for example this PR is only for Android. In that case we can deliver the feature earlier. possibly receive some feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VladislavAntonyuk Do you plan to implement it using the handler approach? Using property mapper and all that jazz?
samples/CommunityToolkit.Maui.Sample/CommunityToolkit.Maui.Sample.csproj
Outdated
Show resolved
Hide resolved
I think it's important to include all platforms in the pull request. Excluding platforms adds complexity and confusion for the users.
💯 All custom controls in the .NET MAUI toolkit must be done using Handlers. We made a commitment to the community that we would always use Handlers and no Renderers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Vlad!
The Windows Sample didn't work on my machine. Can you double check that?
Edit: User error
We also need more unit tests to ensure we've covered every public API on Snackbar
and ISnackbar
.
I also recommend moving the UI elements in Popup.macos.ios.cs
to PopupView.macos.ios.cs
. This'll ensure that Popup
contains the functionality for the popup (eg timer, duration, show/dismiss), and that PopupView
contains all of the UI elements.
Video of Snackbar on Windows Sample App
Here's an update video of the Snackbar
on Windows.
FYI - clicking the Action Button opens a new instance of the app:
src/CommunityToolkit.Maui/Views/Popup/SnackBar/ISnackbar.shared.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui/Views/Popup/SnackBar/NativePopup.ios.macos.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui/Alerts/Snackbar/Snackbar.ios.macos.cs
Outdated
Show resolved
Hide resolved
Update class names, fix padding on apple, attempt to fix winui
@brminnick does this PR implements the pop-up control from XCT or will be a pop-up version of snackbar? |
@pictos, it is a based popup (not specific to snackbar). Just RoundedUIView. |
@VladislavAntonyuk but this will be limited to one action, and a text message, right? Since I was on vacation I probably lost something in the way, so I just to make sure that this doesn't obsolete the pop-up control |
Yup! That @pictos I recommend leveraging We'll be sure to check that the |
Popup class here is just a container. it doesn't contain any code specific to snackbar. PopupView class can be used as a container for any content |
@brminnick I'm working on porting our pop-up to handler. That will take me a few more days, and I'll share a draft PR when I have the android part done. That way we can talk as a team and talk how that should like. I was just concerned if one will overlap other, but looks like that isn't true. I'll take a depper look on this PR asap to be more familiar with the control and architecture |
Looks good to me! Excellent job, @VladislavAntonyuk!! @pictos Do you want to give this one more review before we merge it? |
@brminnick no, we can merge it. And I can rebate against my branch and take a look there. |
Was this ever figured out? I am using snackbar source code as inspiration for a custom implementation but I have the same issue. The MAUI app will just create a new window. |
@emorell96 if I'm not wrong the fix for this is comming (: |
@pictos is it this?
So basically if a new process is started, the idea is to kill it as it starts? Do you know if the events available in UWP are available on MAUI? Usually on UWP you can just override the OnActivated and hook up there in order to handle the activation as shown here: https://docs.microsoft.com/en-us/windows/apps/design/shell/tiles-and-notifications/send-local-toast?tabs=uwp#step-3-handling-activation And if it's different with MAUI, is it possible to use The reason I ask is that I was trying to get this to work with setting the activation type to |
@VladislavAntonyuk do you know the answers for this? |
I just wanted to share a little what I've discovered after quite a bit of experimentation. I thought it might be useful for your nuget. On Windows there's two kind of activations for UWP apps: foreground and background. The former will use the current app but you need to modify the package.appxmanifext file if you want to use For my use case the background task sounded ideal, but I was unable to make it work with MAUI (at least for the In-Process Background tasks). I kept getting argument out of range exception. I might try the out-of-process background tasks later. In any case, one way I found to solve this issue using the foreground activation (the one currently used here), you need to make this change to the in the Package element this needs to be added: xmlns:desktop="http://schemas.microsoft.com/appx/manifest/desktop/windows10"
xmlns:com="http://schemas.microsoft.com/appx/manifest/com/windows10"
IgnorableNamespaces="uap rescap com desktop" Inside of the applications: <Applications>
<Application Id="App" Executable="$targetnametoken$.exe" EntryPoint="$targetentrypoint$">
<uap:VisualElements />
<Extensions>
<!-- Specify which CLSID to activate when toast clicked -->
<desktop:Extension Category="windows.toastNotificationActivation">
<desktop:ToastNotificationActivation ToastActivatorCLSID="guid-used-in-maui-csproj" />
</desktop:Extension>
<!--Register COM CLSID LocalServer32 registry key-->
<com:Extension Category="windows.comServer">
<com:ComServer>
<com:ExeServer Executable="filename-used-for-the-exe-usually-the-assembly-name.exe" Arguments="-ToastActivated" DisplayName="Toast activator">
<com:Class Id="guid-used-in-maui-csproj" DisplayName="Toast activator"/>
</com:ExeServer>
</com:ComServer>
</com:Extension>
</Extensions>
</Application>
</Applications> Using Finally I register in the MauiProgram.cs the event: using Microsoft.Toolkit.Uwp.Notifications;
builder.
... (every other build action)
.ConfigureLifecycleEvents(configure =>
{
configure.AddWindows(c =>
{
c.OnLaunched((window, args) =>
{
ToastNotificationManagerCompat.OnActivated += WindowsNotificationService.OnActivated;
});
});
}); I made this minimal repo that works: Hope it helps. Thanks for the replies! |
@emorell96 do you mind to open a issue and move the last post into it? |
Fix #118