-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix Notifications Overlay that opens automatically #6133
Fix Notifications Overlay that opens automatically #6133
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6133 +/- ##
==========================================
- Coverage 54.65% 54.64% -0.01%
==========================================
Files 600 600
Lines 23950 23950
Branches 2117 2117
==========================================
- Hits 13089 13087 -2
- Misses 10261 10264 +3
+ Partials 600 599 -1
*This pull request uses carry forward flags. Click here to find out more.
Continue to review full report at Codecov.
|
This looks amazing! Would you mind adding a regression test for this area? |
Sure! I will push the test for your review. |
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.
Test looks great! I just had a single question about accessible locators.
Would you mind moving the test into the existing notification.e2e.spec.js file?
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.
Looks great! Just have one minor request and this is good to merge.
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.
Awesome work. Really appreciate the quick turnaround on this! Thank you again.
Closes #6130.
Describe your changes:
Problem
TL;DR The close button does not close properly because of the parent component
We have this current user flow:
true
tofalse
.But after that time, if the user tries to close it, the error occurs.
The emitting event to close of the NotificationsList component is not sent to the NotificationIndicator once it is hidden (not rendered).
Since the variable in charge of displaying the NotificationList component is still
true
, the next time Open MCT tries to send any kind of alert, it will open the alert with the notification list. Here's the bug.Solution
Adding
showNotificationsOverlay
to this condition continue to display the NotificationIndicator element even if there are no notifications. This allows the notification list closing flow to continue on its way without interruption.This has minimal impact on the user experience, because the modal still hides all page content including the indicator itself.
All Submissions:
Author Checklist
Reviewer Checklist