Stacked in-app notification #1921
Conversation
| ContentTemplate = dataTemplate; | ||
| Content = null; | ||
| Show(duration); | ||
| bool shouldDisplayImmediately = ShouldDisplayImmediately(); |
There was a problem hiding this comment.
Could we use Generics or another pattern here to simplify these three different types of template objects? Then the show logic could be centralized rather than triplicated, eh?
There was a problem hiding this comment.
I can surely create a function for it.
docs/controls/InAppNotification.md
Outdated
| You can change this behavior with one of these values: | ||
|
|
||
| * `Replace` - default mode, replace previous notification | ||
| * `QueueBehind` - store every notifications to show, when you dismiss a notification the remaining ones will be displayed successively |
There was a problem hiding this comment.
will be displayed in succession.
Also should this be formatted as a table?
There was a problem hiding this comment.
I can. But this documentation page does not follow the guidelines for the moment.
docs/controls/InAppNotification.md
Outdated
| * `Replace` - default mode, replace previous notification | ||
| * `QueueBehind` - store every notifications to show, when you dismiss a notification the remaining ones will be displayed successively | ||
| * `StackAbove` - store every notifications to show, when you show a notification it will be displayed in priority (in the reverse order of `QueueBehind` mode) | ||
|
|
There was a problem hiding this comment.
Not sure if these names make sense for me. I thought StackAbove at first was going to show multiple notifications on screen at the same time.
Also, shouldn't the be a parameter on the show function vs. a property of the control? Shouldn't I be able to decide that I have a really important message and want it to be next, but usually I'll just queue them up in order?
There was a problem hiding this comment.
Well. I am not sure of the use case but the goal was to mimic the behavior of the Microsoft Edge notifications control. When you have multiple notifications, there is only one visible at a time, and displayed successfully when one is dismissed.
For your 1st question, I think this is not required, a property seems better. For your 2nd question, you can surely change the behavior as you go, from QueueBehind to StackAbove and go back to normal when you want.
There was a problem hiding this comment.
Agree. What about StackInFront/QueueBehind?
| /// <summary> | ||
| /// Base class for information about stacked notification | ||
| /// </summary> | ||
| public abstract class StackedNotificationInfo |
There was a problem hiding this comment.
Maybe have this have a Generic property of ContentHolder?
There was a problem hiding this comment.
Well. How do you handle a type that is not part of the possible types. In F#, you could do that: string | UIElement | DataTemplate but I don't know if it is possible with generics.
There was a problem hiding this comment.
public class StackedNotificationInfo<T>
{
public int Duration { get; set; }
public T ContentHolder { get; set; }
}Then you can do: public void Show<T>(T contentHolder, int duration = 0)
There was a problem hiding this comment.
Is it better than what we have now? I mean, devs could possibly send whatever type they want. We loss the constraints of what I said earlier on restricting to string | UIElement | DataTemplate type.
There was a problem hiding this comment.
Alternatively, you just leave it as object and do pattern matching in the UpdateContent function. If it's not one of the three types you support, either call ToString() or ignore. You have public type safety in the Show methods
There was a problem hiding this comment.
Sure, it is possible. But what I am thinking for devs. Is it better to see a method with 3 overloads (with each recommend type) or a single method where you can insert whatever you want?
I know, this is a small argument but I prefer that we look at every angle. If you are still in favor of the generic version, I'll make the change.
There was a problem hiding this comment.
I don't think we should change the Show methods to take in arbitrary type, they should remain the way they are. I think the discussion here is whether we just need this:
internal class StackedNotificationInfo
{
public int Duration { get; set; }
public object Content { get; set; }
}
vs what you have right now.
There was a problem hiding this comment.
I don't think that's what @michael-hawker wanted. But of course, I can change the shape of the internal class.
There was a problem hiding this comment.
I'm fine with either, the backup behavior @nmetulev suggests makes sense too.
docs/controls/InAppNotification.md
Outdated
| You can change this behavior with one of these values: | ||
|
|
||
| | StackMode properties | Description | | ||
| | -- | -- | -- | |
There was a problem hiding this comment.
You have to remove one | -- |
| /// <summary> | ||
| /// Base class for information about stacked notification | ||
| /// </summary> | ||
| public abstract class StackedNotificationInfo |
There was a problem hiding this comment.
Why is this class public?
|
@michael-hawker, could you review again |
|
Looks pretty good, pinging @shweaver-MSFT though too, as I believe he was just looking at the existing in-app notification templates today. |
|
Based on the current changes I'm seeing, I don't think this will affect the template updates I'm doing. Testing it out currently... |
|
This PR is linked to unclosed issues. Please check if one of these issues should be closed: #1430 |
Issue: #1430
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Show one notification with in-app notification control at a time.
Showing a new notification when an existing is still displayed will replace the previous one.
What is the new behavior?
Add different behavior for stacking notification:
PR Checklist
Please check if your PR fulfills the following requirements:
(https://github.com/Microsoft/UWPCommunityToolkit-design-assets)