Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Notifications UI web view2 #13096

Merged
merged 25 commits into from
Jul 19, 2022
Merged

Notifications UI web view2 #13096

merged 25 commits into from
Jul 19, 2022

Conversation

filipeotero
Copy link
Contributor

Purpose

This PR implements the Notifications UI that is rendered in a Webview component and its HTML created in React (Which in this PR are included just for sample purposes).

image

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 @zeusongit @reddyashish

FYIs

@jesusalvino @RobertGlobant20

QilongTang
QilongTang reviewed Jul 11, 2022
View reviewed changes
src/Notifications/Notifications.csproj Outdated
<None Remove="View\NotificationUI.xaml" />
<None Remove="Web\ArtifaktElement-Regular.woff" />
<None Remove="Web\index.bundle.js" />
<None Remove="Web\index.html" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something we need to work on. I would love to avoid hard copy of the existing component.

RobertGlobant20
RobertGlobant20 reviewed Jul 11, 2022
View reviewed changes
Copy link
Contributor

@RobertGlobant20 RobertGlobant20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments

src/Notifications/Web/index.html Outdated Show resolved Hide resolved
src/DynamoCoreWpf/Controls/ShortcutToolbar.xaml Outdated Show resolved Hide resolved
@QilongTang
Copy link
Contributor

QilongTang commented Jul 11, 2022 •
edited
Loading

@filipeotero Appreciate the implementation!

  1. Is the bundled js file in this PR your implementation or HIG notification flyout?

  2. I also believe we are jumping ahead a bit, I was expecting us to implement something like this https://www.meziantou.net/running-npm-tasks-when-building-a-dotnet-project.htm first to have the files for HIG notification flyout and webview2 ready when building Dynamo. FYI: @RobertGlobant20 @jesusalvino

QilongTang
QilongTang reviewed Jul 11, 2022
View reviewed changes
src/Notifications/NotificationsViewExtension.cs Outdated
{
var dynamoView = (DynamoView)viewLoadedParams.DynamoWindow;
var shortcutBar = dynamoView.ShortcutBar;
var notificationsButton = (Button)shortcutBar.FindName("notificationsButton");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you add a TODO comment to expose button in our extension API?

@filipeotero
Copy link
Contributor Author

@filipeotero Appreciate the implementation!

  1. Is the bundled js file in this PR your implementation or HIG notification flyout?
  2. I also believe we are jumping ahead a bit, I was expecting us to implement something like this https://www.meziantou.net/running-npm-tasks-when-building-a-dotnet-project.htm first to have the files for HIG notification flyout and webview2 ready when building Dynamo. FYI: @RobertGlobant20 @jesusalvino
  1. It's my implementation
  2. Do you mean by this to insert the HIG project on Dynamo and build it with our sln?

@QilongTang QilongTang added this to the 2.16.0 milestone Jul 12, 2022
reddyashish
reddyashish reviewed Jul 12, 2022
View reviewed changes
src/Notifications/NotificationsUIViewModel.cs
/// TODO it should really be responsive either using grid of auto canvas:
/// https://stackoverflow.com/questions/855334/wpf-how-to-make-canvas-auto-resize
/// </summary>
public double PopupRectangleWidth { get; set; } = 310;
Copy link
Contributor

@reddyashish reddyashish Jul 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we calculate this using dynamo window width instead of having a fixed value?

reddyashish
reddyashish reviewed Jul 13, 2022
View reviewed changes
Copy link
Contributor

@reddyashish reddyashish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with couple of comments.

QilongTang
QilongTang reviewed Jul 13, 2022
View reviewed changes
src/Notifications/Web/index.html
</head>
<body>
<div id="root"></div>
<script>#mainJs</script>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want some comments in this file, maybe can be added into the other repo?

QilongTang
QilongTang reviewed Jul 13, 2022
View reviewed changes
Copy link
Contributor

@QilongTang QilongTang left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with some additional comments plus we need the npm install changes (trial npm package by @jesusalvino )

@QilongTang
Copy link
Contributor

Thank you @filipeotero It looks all comments addressed and we are only missing the npm install step

@filipeotero
Copy link
Contributor Author

Thank you @filipeotero It looks all comments addressed and we are only missing the npm install step

Thank you all for the comments! I entered the npm install step, removed the font style file, and added the base64 version directly in the HTML file. Let me know if it's okay.

zeusongit
zeusongit reviewed Jul 15, 2022
View reviewed changes
test/DynamoCoreWpfTests/ViewExtensions/NotificationsExtensionTests.cs Outdated
}

[Test]
public void ValidateNotificationsUIEmbeededFiles()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ValidateNotificationsUIEmbeededFiles spell error, update to - > ValidateNotificationsUIEmbededFiles

@QilongTang
Copy link
Contributor

On a second thought EngOps Build may fail to build because of the reason @mjkkirschner mentioned that internal Jenkins machine has no access to public npm registry

@QilongTang
Copy link
Contributor

@filipeotero Do you know why Dynamo SelfServe failed?

@filipeotero
Copy link
Contributor Author

@filipeotero Do you know why Dynamo SelfServe failed?

It looks like it is failing because of the npm package as well.
I will commit again and comment the part of the npm package to test.

@QilongTang
Copy link
Contributor

@filipeotero Do you know why Dynamo SelfServe failed?

It looks like it is failing because of the npm package as well. I will commit again and comment the part of the npm package to test.

@filipeotero @jesusalvino @RobertGlobant20 @reddyashish @zeusongit In order to unblock this work getting into Dynamo, I would love to stage the commits and put the npm package consumption into another Jira and PR. What do you think?

@reddyashish
Copy link
Contributor

Sounds good to me Aaron.

@QilongTang
Copy link
Contributor

@filipeotero Can you add the hard copy back or simply revert your recent change for now. Otherwise even the mock would not display because the bundled js does not exist in our repo.
image

@filipeotero
Copy link
Contributor Author

@filipeotero Can you add the hard copy back or simply revert your recent change for now. Otherwise even the mock would not display because the bundled js does not exist in our repo. image

Reverted!

@QilongTang QilongTang merged commit 468d604 into DynamoDS:master Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RobertGlobant20 RobertGlobant20 RobertGlobant20 left review comments

@zeusongit zeusongit zeusongit left review comments

@QilongTang QilongTang QilongTang left review comments

@reddyashish reddyashish reddyashish approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.16.0
Development

Successfully merging this pull request may close these issues.
5 participants
@filipeotero @QilongTang @reddyashish @zeusongit @RobertGlobant20