Skip to content

DYN-5296 setting dynamic height for notification center popup#14014

Merged
QilongTang merged 2 commits intoDynamoDS:masterfrom
Enzo707:DYN-5296-setting-dynamic-height-for-notification-center-popup
Jun 6, 2023
Merged

DYN-5296 setting dynamic height for notification center popup#14014
QilongTang merged 2 commits intoDynamoDS:masterfrom
Enzo707:DYN-5296-setting-dynamic-height-for-notification-center-popup

Conversation

@Enzo707
Copy link
Contributor

@Enzo707 Enzo707 commented May 22, 2023 •
edited
Loading

Purpose

Screen.Recording.2023-05-22.at.00.25.32.mov

This PR is related to DYN-5296

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
  • This PR contains no files larger than 50 MB

Release Notes

This PR implements a dynamic height resizing for notifications popup base on Notification Center height.

Reviewers

@RobertGlobant20

FYIs

@RobertGlobant20
@reddyashish

@Enzo707 Enzo707 marked this pull request as draft May 22, 2023 14:49
@QilongTang
Copy link
Contributor

QilongTang commented May 22, 2023

Rebase your branch? @Enzo707

@reddyashish
Copy link
Collaborator

reddyashish commented May 22, 2023

PR on notification center side: DynamoDS/NotificationCenter#36

@reddyashish
Copy link
Collaborator

reddyashish commented May 31, 2023 •
edited
Loading

@Enzo707 Merged the PR on notification center. I know we are waiting on the PR to the HIG repo, but you can clean up this PR. It has conflicts too now. You can just create a new PR on top of updated master, if that is easier as I think this PR has only changes to 2 files.

@QilongTang
Copy link
Contributor

QilongTang commented May 31, 2023

hi @Enzo707 @reddyashish The PR DynamoDS/hig#3 has been merged, right now we only need to publish the npm packages for Dynamo repo to consume

@Enzo707 Enzo707 force-pushed the DYN-5296-setting-dynamic-height-for-notification-center-popup branch from ab11a0c to c93f22f Compare May 31, 2023 19:23
@reddyashish reddyashish marked this pull request as ready for review May 31, 2023 20:13
reddyashish
reddyashish reviewed May 31, 2023
View reviewed changes
src/Notifications/NotificationCenterController.cs
onMarkAllAsRead(ids);
}

public void UpdateNotificationWindowSize(int height)
Copy link
Collaborator

@reddyashish reddyashish May 31, 2023

Choose a reason for hiding this comment

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

Add summary to this public method.

Copy link
Contributor

@QilongTang QilongTang Jun 6, 2023

Choose a reason for hiding this comment

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

For function comments, please use /// while // is mostly used for inline comments for block of code

Copy link
Contributor Author

@Enzo707 Enzo707 Jun 6, 2023

Choose a reason for hiding this comment

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

@QilongTang got it! Thanks

@QilongTang
Copy link
Contributor

QilongTang commented Jun 5, 2023

hi @Enzo707 the reported test failure is a sporadic one which is going to be handled by @avidit , please address the PR comments and we can wrap up this PR

@QilongTang QilongTang added this to the 2.19.0 milestone Jun 5, 2023
@QilongTang
Copy link
Contributor

QilongTang commented Jun 5, 2023

hi @Enzo707 While building your PR locally I did not get the same results, can you double check? Also cleaned up one function that is not used
image

@Enzo707
Copy link
Contributor Author

Enzo707 commented Jun 6, 2023

hi @Enzo707 While building your PR locally I did not get the same results, can you double check? Also cleaned up one function that is not used image

Hi @QilongTang I'm gonna add this function (with a summary) back as this is triggered in NotificationCenter side for passing the current height to Dynamo. The issue that you described above should be resolved after that.

@Enzo707 Enzo707 force-pushed the DYN-5296-setting-dynamic-height-for-notification-center-popup branch from bcdc2d1 to 51a3566 Compare June 6, 2023 15:48
@Enzo707 Enzo707 force-pushed the DYN-5296-setting-dynamic-height-for-notification-center-popup branch from 51a3566 to 81baaad Compare June 6, 2023 16:31
@QilongTang
Copy link
Contributor

QilongTang commented Jun 6, 2023 •
edited
Loading

EngOps build error is a bit unknown, looking for ways to restart it
"E:\Builds\Dynamo_master\Dynamo\src\Tools\Md2Html\Md2Html.csproj" (Rebuild target) (96) ->
(AfterBuildOps target) ->
E:\Builds\Dynamo_master\Dynamo\src\Tools\Md2Html\Md2Html.csproj(41,5): error MSB3073: The command "C:\Users\svc_p_dynamobuild.nuget\packages\ilmerge\3.0.41\build..\tools\net452\ILMerge.exe bin\AnyCPU\Release\Md2Html.exe ^
E:\Builds\Dynamo_master\Dynamo\src\Tools\Md2Html\Md2Html.csproj(41,5): error MSB3073: /allowDup ^
E:\Builds\Dynamo_master\Dynamo\src\Tools\Md2Html\Md2Html.csproj(41,5): error MSB3073: /lib:bin\AnyCPU\Release\ ^
E:\Builds\Dynamo_master\Dynamo\src\Tools\Md2Html\Md2Html.csproj(41,5): error MSB3073: /out:E:\Builds\Dynamo_master\Dynamo\src..\bin\AnyCPU\Release\Md2Html\Md2Html.exe ^
E:\Builds\Dynamo_master\Dynamo\src\Tools\Md2Html\Md2Html.csproj(41,5): error MSB3073: AngleSharp.Css.dll ^
E:\Builds\Dynamo_master\Dynamo\src\Tools\Md2Html\Md2Html.csproj(41,5): error MSB3073: AngleSharp.dll ^
E:\Builds\Dynamo_master\Dynamo\src\Tools\Md2Html\Md2Html.csproj(41,5): error MSB3073: HtmlSanitizer.dll ^
E:\Builds\Dynamo_master\Dynamo\src\Tools\Md2Html\Md2Html.csproj(41,5): error MSB3073: Markdig.dll ^
E:\Builds\Dynamo_master\Dynamo\src\Tools\Md2Html\Md2Html.csproj(41,5): error MSB3073: NDesk.Options.dll ^
E:\Builds\Dynamo_master\Dynamo\src\Tools\Md2Html\Md2Html.csproj(41,5): error MSB3073: System.Buffers.dll ^
E:\Builds\Dynamo_master\Dynamo\src\Tools\Md2Html\Md2Html.csproj(41,5): error MSB3073: System.Memory.dll ^
E:\Builds\Dynamo_master\Dynamo\src\Tools\Md2Html\Md2Html.csproj(41,5): error MSB3073: System.Numerics.Vectors.dll ^
E:\Builds\Dynamo_master\Dynamo\src\Tools\Md2Html\Md2Html.csproj(41,5): error MSB3073: System.Runtime.CompilerServices.Unsafe.dll ^
E:\Builds\Dynamo_master\Dynamo\src\Tools\Md2Html\Md2Html.csproj(41,5): error MSB3073: System.Text.Encoding.CodePages.dll " exited with code 1.

5766 Warning(s)
1 Error(s)

@QilongTang QilongTang merged commit 214d392 into DynamoDS:master Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@reddyashish reddyashish reddyashish left review comments

@QilongTang QilongTang QilongTang approved these changes

@RobertGlobant20 RobertGlobant20 Awaiting requested review from RobertGlobant20

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.19.0

Development

Successfully merging this pull request may close these issues.

3 participants

@Enzo707 @QilongTang @reddyashish