feat(InAppNotifications): improve control #1441

Odonno · 2017-08-26T09:30:51Z

Linked to #1430

WilliamABradley · 2017-08-26T23:13:13Z

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

+        /// <summary>
+        /// When user explicitly dismissed the notification.
+        /// </summary>
+        User = 1,


Any reason why this is 1,2 rather than 0,1?

I copied the behavior of ActivationKind which is a Flag that start from 1. I can revert to the default enum values if necessary.

michael-hawker

Looks like some nice additions. Few minor comments.

michael-hawker · 2017-08-28T16:48:06Z

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

@@ -278,7 +278,10 @@

        <controls:InAppNotification x:Name="ExampleInAppNotification"
                                    Content="This is a test message."
-                                    ShowDismissButton="@[ShowDismissButton:Bool:True]" />
+                                    ShowDismissButton="@[ShowDismissButton:Bool:True]"
+                                    AnimationDuration="@[AnimationDuration:String:100]"


We usually use the Slider type for these sorts of values.

michael-hawker · 2017-08-28T16:48:59Z

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

-                                    ShowDismissButton="@[ShowDismissButton:Bool:True]" />
+                                    ShowDismissButton="@[ShowDismissButton:Bool:True]"
+                                    AnimationDuration="@[AnimationDuration:String:100]"
+                                    VerticalOffset="@[VerticalOffset:String:100]"


You can use the DoubleSlider for these.

michael-hawker · 2017-08-28T16:49:30Z

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

@@ -289,7 +292,10 @@
                                    BorderThickness="0"
                                    ShowDismissButton="False"
                                    VerticalAlignment="Top"
-                                    RenderTransformOrigin="0.5,0">
+                                    RenderTransformOrigin="0.5,0"
+                                    AnimationDuration="@[AnimationDuration:String:100]"


If you reuse the same property in the file later you can just use its name @[AnimationDuration] it'll grab the settings from the first one.

michael-hawker · 2017-08-28T16:54:00Z

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


            Visibility = Visibility.Visible;
            VisualStateManager.GoToState(this, StateContentVisible, true);
+            Opening?.Invoke(this, EventArgs.Empty);


Can we put this before the visibility change and check the eventargs.handled? This way if they register to the event they could prevent the popup from displaying.

@michael-hawker I am not sure how to check the Handled property.

You need to create your own EventArgs class which has a 'Handled' property on it. Then, construct it and pass its reference into the invoke to Opening. Then you can see if the property was set to true afterwards and escape.

Though if we're going to align more with FlyoutBase in the future, we may want to use 'Cancel' as they do here for Closing: https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.controls.primitives.flyoutbaseclosingeventargs

I can directly start with Cancel right now, don't you think?

Yes, sorry, that's kinda what I meant. :P

michael-hawker · 2017-08-28T16:55:22Z

docs/controls/InAppNotification.md

@@ -88,9 +88,50 @@ To hide it, simply set the property to `ShowDismissButton="False"`.

 ## Events

+### Opening
+
+This event is raised when the system or your user started to open of the notification.


The user would never be directly triggering the notification. Maybe:

This event is raised just before the notification starts to open.

michael-hawker · 2017-08-28T16:57:19Z

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

@@ -127,10 +133,25 @@ public void Show(DataTemplate dataTemplate, int duration = 0)
        /// </summary>
        public void Dismiss()
        {
+            Dismiss(InAppNotificationDismissKind.User);


Do we want a third Programmatic one on the enum? As this is the function the developer would call from their code, eh?

michael-hawker · 2017-08-28T17:00:31Z

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

+        /// <summary>
+        /// Event raised when the notification is dismissing
+        /// </summary>
+        public event InAppNotificationDismissingEventHandler Dismissing;


Later we may want to discuss if these should be Closing/Closed to align with FlyoutBase.

Of course. I did not wanted to make a breaking change because the Dismissed event was already available in 2.0.

Yup, makes sense. Just wanted to make sure we have something tracking it for a later release.

We need to decide on this. @nmetulev may have an opinion.

Closed/Closing does fit in better with the Opened/Opening events. If you do want to change it, I recommend marking Dismissed deprecated and adding the new events.

Vijay-Nirmal · 2017-08-29T19:42:24Z

docs/controls/InAppNotification.md

-```xml
+* `AnimationDuration` - duration of the popup animation (in milliseconds)
+* `VerticalOffset` - vertical offset of the popup animation
+* `HorizontalOffset` - horizontal offset of the popup animation


A table would be nice to explain properties. Something like this

Animation properties Type Description

AnimationDuration double Duration of the popup animation in milliseconds

VerticalOffset double Vertical offset of the popup animation

HorizontalOffset double Horizontal offset of the popup animation

Vijay-Nirmal · 2017-08-29T19:42:34Z

docs/controls/InAppNotification.md

-```
+### Using styles
+
+The In App Notification control is designed to support multiple styles. 


Should this be "InAppNotification control"?

"in-app notification" is better.

Vijay-Nirmal · 2017-08-29T19:42:37Z

docs/controls/InAppNotification.md

+### Using styles
+
+The In App Notification control is designed to support multiple styles. 
+The default style applied is the Microsoft Edge-like notification. ??


Is "??" on purpose?

Sorry, it's a mistake.

…scription

Odonno · 2017-08-30T08:51:53Z

I still need to make a fix because the animation properties are not set during initialization.

…hen first initialized

Vijay-Nirmal · 2017-08-31T11:47:21Z

Set default value for VerticalOffset and HorizontalOffset otherwise the app will crash.

Odonno · 2017-08-31T12:28:06Z

@Vijay-Nirmal There is already a default value. VerticalOffset=100 and HorizontalOffset=0.

Vijay-Nirmal · 2017-08-31T12:41:50Z

@Odonno If I didn't set VerticalOffset and HorizontalOffset then I am getting exceptions.

Setting default value for VerticalOffset and HorizontalOffset in InAppNotification.xaml solves my problem

<Setter Property="VerticalOffset" Value="100"/>
<Setter Property="HorizontalOffset" Value="0"/>

Odonno · 2017-08-31T18:41:16Z

@Vijay-Nirmal I thought I added them in the default template. Sorry for this mistake. Currently fixing it.

…template

nmetulev · 2017-09-28T16:41:28Z

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

+        /// <summary>
+        /// Event raised when the notification is dismissing
+        /// </summary>
+        public event InAppNotificationDismissingEventHandler Dismissing;


Closed/Closing does fit in better with the Opened/Opening events. If you do want to change it, I recommend marking Dismissed deprecated and adding the new events.

nmetulev · 2017-09-28T16:49:18Z

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

+            }
+        }
+
+        private void UpdateAnimationDuration(int duration)


What happens if I've already re-templated the control and added custom duration? Or what if I want to have multiple DoubleAnimationUsingKeyFrames with different durations. Seems like this will break re-templated controls.

Yes. It's possible. I don't know what is the best strategy to have default animations.

I'd try to see if you can use TemplateBinding in the storyboard in the template itself. That should allow you to remove all of this code and have everything in the template.

nmetulev · 2017-09-28T16:51:22Z

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

+        }
+
+        private void UpdateVerticalOffset(double verticalOffset)
+        {


This is also seems like it will be messing with my custom animations? Instead of doing the Update(Animation/Offset) through code, is it possible to instead do this through TemplateBinding in the template itself?

…hem with Close events

nmetulev · 2017-10-24T16:51:20Z

ping @Odonno

Odonno · 2017-10-24T17:46:33Z

@nmetulev Sorry. Not enough time right now. Will work on it as soon as I can.

…osoft/UWPCommunityToolkit into inAppNotificationsNmetulev

Vijay-Nirmal · 2017-11-04T14:43:03Z

Use > [!NOTE] in InAppNotification.md/#24 as per #issuecomment-341176267

nmetulev

This a great addition to the control. With the exception of minor changes, looks good to me.

nmetulev · 2017-11-07T17:53:16Z

Microsoft.Toolkit.Uwp.SampleApp/readme.md

@@ -38,6 +38,7 @@ You can define "interactive" values in this file. The value types can be:
 * String: You want the user to provide a text. The string is built like this @[Name:**String**:Default value]
 * Slider: You want the user to provide a double value. The string is built like this @[Name:**Slider**:Default value:min-max]
 * DoubleSlider: Same as slider but with double values (0.01 precision)
+* TimeSpan: You want the user to a duration. The string is built like this (all values in miliseconds) @[Name:**TimeSpan**:DefaultValue:min-max]


Missing a word, should be You want the user to provide a duration

nmetulev · 2017-11-07T18:01:04Z

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

+        /// <summary>
+        /// Event raised when the notification is closed
+        /// </summary>
+        public event EventHandler Closed;


Should the Closed EventHandler also provide the details on how it was closed?

@nmetulev Well, it can a little bit harder for this part. I used a Timer for both Opened and Closed events, started after the Opening and Closing events respectively. I am not sure if it's possible but I can eventually store the InAppNotificationDismissKind before Tick event. What do you think?

Yeah, that makes sense to me, you can store the last dismisskind in a variable

nmetulev · 2017-11-07T18:06:20Z

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

+        /// Event raised when the notification is dismissing
+        /// </summary>
+        [Obsolete("Use Closing event instead.")]
+        public event InAppNotificationDismissingEventHandler Dismissing;


No need to add this event if it wasn't there before

nmetulev · 2017-11-07T18:06:41Z

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

@@ -0,0 +1,48 @@
+// ******************************************************************


I think this class can be removed with the event.

Odonno · 2017-11-07T22:19:52Z

@nmetulev Should be good.

nmetulev · 2017-11-07T22:24:32Z

Now we just need @michael-hawker seal of approval :)

michael-hawker · 2017-11-07T22:41:20Z

docs/controls/InAppNotification.md

@@ -21,27 +21,28 @@ The control should be placed where you want your notification to be displayed in

 ```

-**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.
+> [!NOTE]


What do we do for other notes? We should be consistent still.

We are moving to use this for all the docs per #1569

Sorry, missed that note :), all good!

michael-hawker

Looks great, thanks! Small question about the doc formatting, otherwise can't wait to try out the improvements!

feat(InAppNotifications): add events Opening, Opened and Dismissing

844a176

dnfclas added the cla-already-signed label Aug 26, 2017

Odonno added controls 🎛️ improvements ✨ labels Aug 26, 2017

Odonno added 2 commits August 26, 2017 11:35

docs(InAppNotifications): add docs on new events

4af5fb8

feat(InAppNotifications): add details on Dismissing event args

348ab1c

nmetulev added this to the v2.1 milestone Aug 26, 2017

WilliamABradley reviewed Aug 26, 2017

View reviewed changes

feat(InAppNotifications): improve animations

4514004

Odonno added the need documentation 📃 label Aug 27, 2017

michael-hawker requested changes Aug 28, 2017

View reviewed changes

Odonno added 5 commits August 28, 2017 22:50

refactor(InAppNotifications): split styles into separate files

13dbd44

refactor(InAppNotifications): improve docs and code

8b2722a

feat(InAppNotifications): add ability to cancel the opening/closing

a8965f6

docs(InAppNotifications): update animation part

27539f5

docs(InAppNotifications): add styling part

562ac87

Odonno added reviewers wanted and removed need documentation 📃 labels Aug 29, 2017

Vijay-Nirmal reviewed Aug 29, 2017

View reviewed changes

docs(InAppNotifications): fix typos and improve animation property de…

9676d3b

…scription

Odonno mentioned this pull request Aug 30, 2017

Opening/Opened/Dismissed events support for InAppNotification. #1454

Closed

Odonno added 2 commits August 30, 2017 19:45

merge

fb86ea9

fix(InAppNotifications): update animation properties on the control w…

ad14b9b

…hen first initialized

fix(InAppNotifications): add default values for animation in default …

227619a

…template

nmetulev changed the base branch from dev to master September 1, 2017 04:03

nmetulev reviewed Sep 28, 2017

View reviewed changes

Odonno and others added 2 commits September 30, 2017 11:28

feat(InAppNotification): mark Dimiss events as obsolete and replace t…

828c517

…hem with Close events

Moved animation properties to template

99ccc76

nmetulev added work in progress and removed reviewers wanted labels Oct 24, 2017

msftgits removed the cla-already-signed label Oct 26, 2017

Odonno added 4 commits November 4, 2017 11:28

merge

31d2568

Merge branch 'nmetulev/InAppNotifications' of https://github.com/Micr…

0b0dfb2

…osoft/UWPCommunityToolkit into inAppNotificationsNmetulev

docs(InAppNotification): update code blocks

1178f6b

docs(InAppNotification): change type of AnimationDuration property

f5e38ef

Odonno added reviewers wanted and removed work in progress labels Nov 4, 2017

docs(InAppNotification): use Note block

fb5282f

nmetulev requested changes Nov 7, 2017

View reviewed changes

Odonno added 4 commits November 7, 2017 21:37

docs(SampleApp): fix docs on TimeSpan

2093c1a

refactor(InAppNotification): remove Dismissing event

b8c5e5f

docs(InAppNotification): add missing header

33a1c3e

feat(InAppNotification): add dismiss kind info on closed event

f9ec93a

nmetulev approved these changes Nov 7, 2017

View reviewed changes

michael-hawker reviewed Nov 7, 2017

View reviewed changes

michael-hawker approved these changes Nov 7, 2017

View reviewed changes

nmetulev merged commit 0b06b20 into CommunityToolkit:master Nov 7, 2017

Odonno deleted the inAppNotificationImprovements branch November 8, 2017 07:53

Animation properties	Type	Description
`AnimationDuration`	double	Duration of the popup animation in milliseconds
`VerticalOffset`	double	Vertical offset of the popup animation
`HorizontalOffset`	double	Horizontal offset of the popup animation

		@@ -0,0 +1,48 @@
		// ******************************************************************

feat(InAppNotifications): improve control #1441

feat(InAppNotifications): improve control #1441

Conversation

Odonno commented Aug 26, 2017

Choose a reason for hiding this 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

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 30, 2017

Vijay-Nirmal commented Aug 31, 2017

Odonno commented Aug 31, 2017

Vijay-Nirmal commented Aug 31, 2017

Odonno commented Aug 31, 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

Choose a reason for hiding this comment

nmetulev commented Oct 24, 2017

Odonno commented Oct 24, 2017

Vijay-Nirmal commented Nov 4, 2017

nmetulev 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

Odonno commented Nov 7, 2017

nmetulev commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-hawker left a comment

Choose a reason for hiding this comment