| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 658
DYN-5296 setting dynamic height for notification center popup #14014
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.
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
DYN-5296 setting dynamic height for notification center popup #14014
Conversation
|
Rebase your branch? @Enzo707 |
|
PR on notification center side: DynamoDS/NotificationCenter#36 |
|
@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. |
|
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
force-pushed
the
DYN-5296-setting-dynamic-height-for-notification-center-popup
branch
from
ab11a0c to
c93f22f
Compare
| } | ||
|
|
||
| public void SetNoficationsAsRead(object[] ids) | ||
| { | ||
| onMarkAllAsRead(ids); | ||
| } | ||
|
|
||
| public void UpdateNotificationWindowSize(int height) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@QilongTang got it! Thanks
|
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 |
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
force-pushed
the
DYN-5296-setting-dynamic-height-for-notification-center-popup
branch
from
bcdc2d1 to
51a3566
Compare
Enzo707
force-pushed
the
DYN-5296-setting-dynamic-height-for-notification-center-popup
branch
from
51a3566 to
81baaad
Compare
|
EngOps build error is a bit unknown, looking for ways to restart it |
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
*.resxfilesRelease Notes
This PR implements a dynamic height resizing for notifications popup base on Notification Center height.
Reviewers
@RobertGlobant20
FYIs
@RobertGlobant20
@reddyashish