DYN-5190 mark all as read by filipeotero · Pull Request #13260 · DynamoDS/Dynamo

filipeotero · 2022-08-26T21:51:30Z

Purpose

This PR contains the functionality of marking all notifications as read.

Added a new property to the schema to define if the notification is read
The list notification ids are being stored in the preferences settings
Creation of the scriptObject class that communicates from js to c# code

This PR depends on the following implementation: DynamoDS/NotificationCenter#26

Declarations

Check these if you believe they are true

The codebase is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.
This PR modifies some build requirements and the readme is updated

Reviewers

@QilongTang @reddyashish @zeusongit

FYIs

@RobertGlobant20 @jesusalvino

QilongTang · 2022-08-29T14:54:11Z

Hi @filipeotero From your gif, the Notification fly out height is longer than the panel itself with quite a gap, can you reduce the height to match the size?

src/DynamoCore/Configuration/PreferenceSettings.cs

src/DynamoCoreWpf/ViewModels/Core/ShortcutToolbarViewModel.cs

src/Notifications/NotificationsModel.cs

src/Notifications/NotificationCenterController.cs

QilongTang

Some comments then LGTM

QilongTang · 2022-08-29T15:45:47Z

Currently seems no time diff is considered, but can we add a hard limit? Just in case, that setting was wiped out in certain cases.. @Amoursol What would be an idea threshold? 6 months? 3 months?

filipeotero · 2022-08-29T15:47:56Z

Hi @filipeotero From your gif, the Notification fly out height is longer than the panel itself with quite a gap, can you reduce the height to match the size?

Hi Aaron, I noticed that this extra space in the bottom comes from the high component. The panel is calculating that space but I didn't update the number after inserting the footer. I will update the code and create a PR that will fix it.

reddyashish · 2022-08-29T16:02:23Z

@filipeotero Another thing I just noticed is, when Dynamo is maximized, the notification bell is placed close to the right corner and the notification panel anchor is off by a distance. Do you think you can look into it? or should we create a new task for it?
@QilongTang @Amoursol Also what do you guys think about the blue square border that we show when you click on the notification bell? I think it would be better without that.

QilongTang

LGTM. The only one last question is time limit threshold which can be another task or PR.

QilongTang · 2022-08-29T19:06:45Z

@filipeotero Another thing I just noticed is, when Dynamo is maximized, the notification bell is placed close to the right corner and the notification panel anchor is off by a distance. Do you think you can look into it? or should we create a new task for it? @QilongTang @Amoursol Also what do you guys think about the blue square border that we show when you click on the notification bell? I think it would be better without that.

I think me and @RobertGlobant20 looked at that blue hover over background but we did not figure out how to get rid of it. @RobertGlobant20 any insight?

…tton

filipeotero · 2022-08-29T20:33:10Z

@filipeotero Another thing I just noticed is, when Dynamo is maximized, the notification bell is placed close to the right corner and the notification panel anchor is off by a distance. Do you think you can look into it? or should we create a new task for it? @QilongTang @Amoursol Also what do you guys think about the blue square border that we show when you click on the notification bell? I think it would be better without that.

Hi @reddyashish! I added some space in the notifications button and an offset at the pointer to fit on the screen:

filipeotero · 2022-08-29T21:10:39Z

Hi @filipeotero From your gif, the Notification fly out height is longer than the panel itself with quite a gap, can you reduce the height to match the size?

Hi @filipeotero From your gif, the Notification fly out height is longer than the panel itself with quite a gap, can you reduce the height to match the size?

Hi Aaron, I noticed that this extra space in the bottom comes from the high component. The panel is calculating that space but I didn't update the number after inserting the footer. I will update the code and create a PR that will fix it.

PR was created to fix the panel height

DynamoDS/hig#2

Amoursol · 2022-08-29T21:16:29Z

Currently seems no time diff is considered, but can we add a hard limit? Just in case, that setting was wiped out in certain cases.. @Amoursol What would be an idea threshold? 6 months? 3 months?

6 months is ideal given the slow news cadence, but 3 months is OK too if you need to.

Amoursol · 2022-08-29T21:17:19Z

@filipeotero Another thing I just noticed is, when Dynamo is maximized, the notification bell is placed close to the right corner and the notification panel anchor is off by a distance. Do you think you can look into it? or should we create a new task for it? @QilongTang @Amoursol Also what do you guys think about the blue square border that we show when you click on the notification bell? I think it would be better without that.

Ideally we don't have the blue hover color, as this goes against the rest of our patterns. We should instead have a blue icon on hover.

QilongTang · 2022-08-29T22:28:09Z

There is an regression from this PR, DynamoCoreWpfTests.DynamoViewTests.OpeningWorkspaceWithTrustWarning can you confirm if this is real?

QilongTang · 2022-08-29T23:02:07Z

@filipeotero Another thing I just noticed is, when Dynamo is maximized, the notification bell is placed close to the right corner and the notification panel anchor is off by a distance. Do you think you can look into it? or should we create a new task for it? @QilongTang @Amoursol Also what do you guys think about the blue square border that we show when you click on the notification bell? I think it would be better without that.

Ideally we don't have the blue hover color, as this goes against the rest of our patterns. We should instead have a blue icon on hover.

@filipeotero This should be a quick one, when Dynamo grabs notifications, can we ignore notifications from 6 month ago?

filipeotero · 2022-08-30T13:47:06Z

There is an regression from this PR, DynamoCoreWpfTests.DynamoViewTests.OpeningWorkspaceWithTrustWarning can you confirm if this is real?

The tests are passing locally:

filipeotero · 2022-08-30T14:13:59Z

src/Notifications/NotificationCenterController.cs

                jsonStringFile = reader.ReadToEnd();
                notificationsModel = JsonConvert.DeserializeObject<NotificationsModel>(jsonStringFile);
+
+                //We are adding a limit of months to grab the notifications


@QilongTang

QilongTang · 2022-08-30T15:39:04Z

@filipeotero Would you cherry-pick this PR?

* mark all as read function * mark all as read functionallity * remove local settings * Addressing comments * updating the pointer position and add a margin to the notifcations button * Limit of months to filter the notifications

filipeotero added 4 commits August 24, 2022 14:42

mark all as read function

a3557d2

merge from master

e944786

mark all as read functionallity

beeac0e

remove local settings

c02a6a8

reddyashish approved these changes Aug 29, 2022

View reviewed changes