feat(InAppNotification): first version of the control #1343

Odonno · 2017-07-25T11:44:06Z

Linked to #1080

First version of the In App Notification control. The control is very flexible : devs can update Background color, Border, Padding, Margin. By default, the control looks like the Microsoft Edge in-app notification.

Need more things before merging :

Animations
Add ability to use DataTemplate
~~Add some default DataTemplates (just Text, Yes/No buttons, ...) and should be responsive (Adaptative design)~~ EDIT : cannot make default DataTemplate because they require code-behind variables (text value, click event or command button)
Documentation
Dismiss button should be optional (DependencyProperty)
Add more examples (with DropShadow, vscode-like notification)
Add duration

…buttons

skendrot · 2017-07-26T17:18:08Z

Should this follow the ContentDialog pattern and have PrimaryButton* and SecondaryButton*?

skendrot · 2017-07-26T17:21:30Z

Should this be a type of Flyout? eg: NotificationFlyout

skendrot · 2017-07-26T17:25:12Z

I would expect a control like this to be similar to Toastr

Odonno · 2017-07-26T17:26:53Z

@skendrot I don't know. It could have one button, two buttons, three buttons, no button, images, whatever. I suppose we are replicating the behavior of Toast notification in some sort of way.

Odonno · 2017-07-26T17:27:53Z

Yes, looks pretty much like a Flyout but not sure we can inherit from it.

skendrot · 2017-07-26T17:30:45Z

Flyout is not sealed and has a public ctor, so shouldn't be a problem

Odonno · 2017-07-26T17:33:36Z

You can perfectly create almost anything that you see in toastr. Like I said, my implementation came from Microsoft Edge in-app notification.

skendrot · 2017-07-26T17:37:46Z

What do you mean? I don't see a way to say it's a warning message or to set the duration of the notification or many other features

Odonno · 2017-07-26T17:51:42Z

@skendrot You can update properties the way you want :

Background color for warning or error message
Vertical and Horizontal alignment to set the position
Call Dismiss using a Timer for notification duration

Of course, these features are not natives and devs have to deal with it. Unless someone have a different opinion. :)

skendrot · 2017-07-26T18:06:36Z

Not exactly what I was referring too 😆

Odonno · 2017-07-26T18:21:36Z

One question: Can we add DropShadow on a Flyout? Or ContentDialog?

skendrot · 2017-07-26T19:04:37Z

For a flyout you define the flyout presenter. I'm sure you could do it in there.

tibitoth · 2017-07-28T09:18:45Z

repost my question here:

Can the design of in-app notification be like in the Visual Studio Code?

…n samples

Odonno · 2017-08-12T12:06:49Z

@nmetulev It should work now.

Vijay-Nirmal · 2017-08-12T14:29:04Z

@Odonno I found two issues

Doesn't work properly in StackPanel. I understand the reason for that. Maybe you should recommend the dev to use Grid instead of StackPanel in the Doc
How to set the InAppNotification to Pop from Top to Bottom instead of Bottom to Top?

Odonno · 2017-08-12T15:20:43Z

@Vijay-Nirmal

Yes. I work with the Grid because it is the the main UI container in Windows XAML application. The thing is that devs can put the control everywhere so if they want to put it in a StackPanel of 700px Width and 200px height, they can. It may not be the most efficient way to use this control it is one of the many potential uses of the control. Not sure if adding a word on this is useful.
If you want to change the animation, you'll have to change the RenderTransformOrigin property. By default, it is Bottom to Top but you can invert Y value so it goes in the opposite direction. Check the vscode-like example.

Vijay-Nirmal · 2017-08-12T15:55:25Z

@Odonno I think you can add RenderTransformOrigin method to change Pop Up direction in the Doc (OR) Dedicated property would be nice

Odonno · 2017-08-12T18:45:27Z

@Vijay-Nirmal I would like a Property but not sure how to do it. The RenderTransformOrigin allows me to set animation the way I want. But it may not be the best option.

I can add a description on how to change the animation using RenderTransformOrigin in the docs.

nmetulev

I noticed the animation does not run the first time the control is shown, it just becomes visible, could you take a look?

I tried the sample code (with template) from the code bind file in a blank new app and the app crashed. I generated the nugets and used them in the app to test.

nmetulev · 2017-08-17T05:24:29Z

Microsoft.Toolkit.Uwp.UI.Controls/InAppNotification/InAppNotification.cs

+ }
+
+ _dismissButton = (Button)GetTemplateChild(DismissButtonPart);
+ _dismissButton.Visibility = ShowDismissButton ? Visibility.Visible : Visibility.Collapsed;


Should this move in the null check block below?

That might be better. :)

nmetulev · 2017-08-17T05:35:45Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/InAppNotification/InAppNotificationCode.bind

+ </VisualStateGroup>
+ </VisualStateManager.VisualStateGroups>
+
+ <Grid x:Name="RootGrid" SizeChanged="RootGrid_SizeChanged">


Why the SizeChanged event?

Do not remember, I think this is a old version. Will update it.

Ok. I remember. I made it that way to be able to detect the change of the Grid Width (not the App Width) so I can create a StateTrigger based on the container Size, not the App Size.

I do not know if there is a better option because it will crash if you do not have this code in your code-behind:

public bool IsRootGridActualWidthLargerThan700 { get; set; } private void RootGrid_SizeChanged(object sender, SizeChangedEventArgs e) { // When the root part size of the In App Notification template changed, we should apply VisualState bool newValue = e.NewSize.Width > 700; if (IsRootGridActualWidthLargerThan700 != newValue) { IsRootGridActualWidthLargerThan700 = newValue; OnPropertyChanged(nameof(IsRootGridActualWidthLargerThan700)); } }

Is that required for an example? Should it work without it?

To be clear, the DataTemplate available in the bind code file is a specific template that handle responsive design based on the container Width. And to make it work, I need a property that tell what StateTrigger I should use, in this case I used a boolean in the code behind.

Of course, it is not the default behavior and you can perfectly use the control without extra code :

<controls:InAppNotification x:Name="ExampleInAppNotification" />

And then use .ShowAsync() method.

My objective with the DataTemplate was to provide a more complete example that mimic MS Edge notification with buttons (Horizontal Orientation when Width > 700px, Vertical Orientation otherwise).

Should I remove this template from the code file?

My thoughts on this are to have a simple template that doesn't complicate the mechanics of the control and shows the minimum needed to try the control. We could always have both?

@nmetulev I can leave the complete Templates in the page (with buttons, with DropShadow, etc..) and add a simple template in the code file. I'll update it.

Odonno · 2017-08-17T17:43:31Z

@nmetulev Another question: Is it interesting and how can we integrate Styles (like the vscode-like notification template) so developers can reuse them without copying the code everytime in their apps? My long-term goal would be to gather some styles devs are used to work with (like MS Edge-like, vscode-like and some more to come).

…ty is toggled

nmetulev · 2017-08-18T19:39:05Z

@Odonno, I agree, it would be interesting to provide common style after the control is available to see what requests devs have

Vijay-Nirmal · 2017-08-19T07:03:22Z

docs/controls/InAppNotification.md

+
+## Syntax
+
+The control should be placed where you want your notification to be displayed in the page, generally in the root grid.


I think we should tell the dev to put this control in the Bottom of the grid because If they put at the top other controls will overlay this control.

Or use the ol' Canvas.ZIndex="99999"

or int.MaxValue

I think Canvas.ZIndex has a limit below Int.MaxValue, possibly 16 Bit, I remember, because once I entered a few too many 9s. Unless I added 11 9s.

@Vijay-Nirmal By default, the position is set to Bottom because I mimic the original component from MS Edge. But of course, you can place it at the Top or at the Center, it depends on the dev intention.

@Odonno What do you mean by "By default, the position is set to Bottom"?
I think we can set Canvas.ZIndex by default because Notification will always be displayed about all controls. If devs want to change it they can easily change it by setting Canvas.ZIndex to 0.

@Vijay-Nirmal If you put the control at the last position of the parent container, it will be have priority over other controls, if you do not use Canvas.ZIndex. I can add this to the description of the control if you think it is relevant.

@Odonno I think we should either say it in Doc or set a default Canvas.ZIndex value

We can always add a note in the docs if that will make things easier. How about:

Note: Since the control is part of the page visual tree, it will render in the order it was added in the parent control, and might be hidden by other elements. For the control to render on top of other elements, add it as the last child of the parent control or set the Canvas.ZIndex to a high number.

…ve DataTemplate

IbraheemOsama · 2017-08-22T01:09:22Z

Will review this

IbraheemOsama · 2017-08-22T01:36:06Z

Microsoft.Toolkit.Uwp.UI.Controls/InAppNotification/InAppNotification.cs

+ DefaultStyleKey = typeof(InAppNotification);
+ }
+
+ protected override void OnApplyTemplate()


missing documentations /// <inheritdoc />

IbraheemOsama · 2017-08-22T01:38:10Z

Microsoft.Toolkit.Uwp.UI.Controls/InAppNotification/InAppNotification.cs

+ /// <param name="text">Text used as the content of the notification</param>
+ /// <param name="duration">Displayed duration of the notification in ms (less or equal 0 means infinite duration)</param>
+ /// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns>
+ public async Task ShowAsync(string text, int duration = 0)


This function could be done this way
public Task ShowAsync(string text, int duration = 0) { ContentTemplate = null; Content = text; return ShowAsync(duration); }
It's better because you're giving more control to the caller when to await instead of forcing the await in your function.
also this is applied to the rest of ShowAsync

except of the first ShowAsync :) you can't do that to it

Seems fair.

IbraheemOsama

The on applyTemplate is missing the documentations.
regarding the async functions feel free to do it or not :) but it's just a better approach.

…ShowAsync methods

michael-hawker

Excited to see this first version go in. I think if we want the new sample app to work we'll need the opened event though.

michael-hawker · 2017-08-22T20:43:58Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/InAppNotification/InAppNotificationPage.xaml

+ <TextBlock Text="*if duration is less or equal 0, the notification will never be dismissed" />
+ </StackPanel>
+
+ <Button x:Name="ShowNotificationWithRandomTextButton" 


These types of buttons in the sample should instead be registered as commands with Shell.Current.RegisterNewCommand.

I don't know how to do this. From what sample control should I take inspiration?

Take a look at this. It simply adds a button in the shell instead of having to add one in your sample.

Thanks. I'm on it.

michael-hawker · 2017-08-22T20:44:16Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/InAppNotification/InAppNotificationPage.xaml.cs

+ ExampleVSCodeInAppNotification.Dismiss();
+ SetDefaultControlTemplate();
+
+ var grid = new Grid();


Can you add this as a template like the other examples?

This is a large example and templates work a lot better in the new live XAML world.

I wanted to show different examples : so there is one with XAML DataTemplate and another one using C#.

People using the app won't see this code though, should it be in the .bind Code File instead?

michael-hawker · 2017-08-22T21:04:48Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/InAppNotification/InAppNotificationXaml.bind

+
+ <StackPanel Grid.Column="2" Orientation="Horizontal">
+ <Button Content="Action 1" 
+ Click="Action1Button_Click"


Trying to think how this click event will work in the new sample app...

Since these are more geared to individual notifications vs a central service, I'd like to see an Opened/Opening event like FlyoutBase has. Then we can grab the buttons in that event and register handlers for the sample app.

michael-hawker · 2017-08-22T21:05:58Z

Microsoft.Toolkit.Uwp.UI.Controls/InAppNotification/InAppNotification.Events.cs

+ /// <summary>
+ /// In App Notification defines a control to show local notification in the app.
+ /// </summary>
+ public partial class InAppNotification


Could we also have events for Opened/Opening/Closed/Closing like ContentDialog / FlyoutBase?

I would see closed/closing fire whenever it disappears (including dismiss or timeout) vs. the Dismiss event which fires when the user explicitly dismisses the notification.

I need to figure out how to inherits from Flyout/FlyoutPresenter. For the dismiss event, I can easily add an argument whether the user clicked on a button or if timeout.

Don't think there is a need to inherit from Flyout. You can add custom events. PullToRefeshListView events come to mind as an example.

…e app

Odonno · 2017-08-23T18:14:58Z

@nmetulev I leave it to you.

feat(InAppNotification): first version of the control

24d0431

Odonno added controls 🎛️ need documentation 📃 labels Jul 25, 2017

Odonno added this to the v2.0 milestone Jul 25, 2017

Odonno requested review from nmetulev and skendrot July 25, 2017 11:44

dnfclas added the cla-already-signed label Jul 25, 2017

Odonno added 5 commits July 25, 2017 14:24

feat(InAppNotification): add animations

a712c30

feat(inAppNotification): add ability to use DataTemplate

2babc22

feat(InAppNotification): set responsive design for DataTemplate with …

b9709df

…buttons

docs(InAppNotification): add bind files

a201546

docs(InAppNotification): add markdown file

019a3dd

Odonno added documentation done and removed need documentation 📃 labels Jul 26, 2017

docs(InAppNotification): add link from readme file

e142ec4

nmetulev added the reviewers wanted label Jul 26, 2017

feat(InAppNotification): add ability to hide dismiss button

208a650

docs(InAppNotification): add ability to show or hide dismiss button i…

1ecc520

…n samples

nmetulev requested changes Aug 17, 2017

View reviewed changes

fix(InAppNotification): set dismiss button visiblity after null check

b78f3d4

Odonno added 2 commits August 17, 2017 19:49

docs(InAppNotification): add popup animation explanation

8846f3d

fix(InAppNotification): set correct animation the first time visibili…

0a3c40d

…ty is toggled

Vijay-Nirmal reviewed Aug 19, 2017

View reviewed changes

Odonno added 2 commits August 21, 2017 22:30

docs(InAppNotifications): add note about the render priority

2b7ac18

fix(InAppNotifications): simplify code bind file using a non responsi…

ac85365

…ve DataTemplate

nmetulev approved these changes Aug 21, 2017

View reviewed changes

IbraheemOsama reviewed Aug 22, 2017

View reviewed changes

IbraheemOsama suggested changes Aug 22, 2017

View reviewed changes

Odonno added 2 commits August 22, 2017 20:30

merge

8fe1df5

refactor(InAppNotifications): use return instead of async keyword on …

f4c6902

…ShowAsync methods

michael-hawker requested changes Aug 22, 2017

View reviewed changes

docs(InAppNotifications): use Shell to register commands in the sampl…

5fb2211

…e app

michael-hawker approved these changes Aug 23, 2017

View reviewed changes

IbraheemOsama approved these changes Aug 23, 2017

View reviewed changes

IbraheemOsama merged commit 2fa5a16 into CommunityToolkit:dev Aug 23, 2017

Odonno deleted the inAppNotifications branch August 24, 2017 09:42

michael-hawker mentioned this pull request May 6, 2020

Proposal: UI for Long-Lived App-Wide Status Messages microsoft/microsoft-ui-xaml#913

Closed


		## Syntax

		The control should be placed where you want your notification to be displayed in the page, generally in the root grid.

feat(InAppNotification): first version of the control #1343

feat(InAppNotification): first version of the control #1343

Uh oh!

Conversation

Odonno commented Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

skendrot commented Jul 26, 2017

skendrot commented Jul 26, 2017

skendrot commented Jul 26, 2017

Odonno commented Jul 26, 2017

Odonno commented Jul 26, 2017

skendrot commented Jul 26, 2017

Odonno commented Jul 26, 2017

skendrot commented Jul 26, 2017

Odonno commented Jul 26, 2017

skendrot commented Jul 26, 2017

Odonno commented Jul 26, 2017

skendrot commented Jul 26, 2017

tibitoth commented Jul 28, 2017

Odonno commented Aug 12, 2017

Vijay-Nirmal commented Aug 12, 2017

Odonno commented Aug 12, 2017

Vijay-Nirmal commented Aug 12, 2017

Odonno commented Aug 12, 2017

nmetulev left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Odonno Aug 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Odonno commented Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

nmetulev commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vijay-Nirmal Aug 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vijay-Nirmal Aug 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vijay-Nirmal Aug 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IbraheemOsama commented Aug 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IbraheemOsama left a comment

Choose a reason for hiding this comment

michael-hawker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Odonno commented Aug 23, 2017

Labels

9 participants

Odonno commented Jul 25, 2017 •

edited

Loading

nmetulev left a comment •

edited

Loading

Odonno Aug 19, 2017 •

edited

Loading

Odonno commented Aug 17, 2017 •

edited

Loading

Vijay-Nirmal Aug 19, 2017 •

edited

Loading

Vijay-Nirmal Aug 19, 2017 •

edited

Loading

Vijay-Nirmal Aug 19, 2017 •

edited

Loading