Chore: Setup viewer notifications in the correct location #563
Conversation
|
Verified that @pramodsum has signed the CLA. Thanks for the pull request! |
src/lib/PreviewUI.js
Outdated
| @@ -299,6 +310,10 @@ class PreviewUI { | |||
| * @return {void} | |||
| */ | |||
| showNotification(message, buttonText) { | |||
| if (!this.notification) { | |||
| this.setupNotification(); | |||
| } | |||
There was a problem hiding this comment.
Good idea moving it here! One thought - you have a null check inside setupNotification. Is there a situation where contentContainer doesn't exist and then someone calls showNotification? This would fall into the first if block here but then fail on this.notifciation.show()
There was a problem hiding this comment.
Easiest fix would be:
if (!this.notification) {
this.setupNotification();
}
if (this.notification) {
this.showNotification();
}
But, I'm not a huge fan of how that reads... Open to other suggestions.
There was a problem hiding this comment.
cleaned it up a bit. Thoughts?
src/lib/PreviewUI.js
Outdated
| @@ -309,6 +324,10 @@ class PreviewUI { | |||
| * @return {void} | |||
| */ | |||
| hideNotification() { | |||
| if (!this.notification) { | |||
| this.setupNotification(); | |||
| } | |||
There was a problem hiding this comment.
I don't think you need to setup notification here since it's only a prerequisite to showing a notification.
Something like this should work:
if (this.notification) {
this.notification.hide();
}
src/lib/PreviewUI.js
Outdated
| @@ -264,9 +264,6 @@ class PreviewUI { | |||
| // part of finishLoadingSetup in BaseViewer.js | |||
| crawler.classList.remove(CLASS_HIDDEN); | |||
| } | |||
|
|
|||
| // Setup viewer notification | |||
| this.notification = new Notification(this.contentContainer); | |||
| } | |||
There was a problem hiding this comment.
Since you're in this file already, I'd move this check to be:
if (!this.contentContainer) {
return;
}
to reduce arrow-ness
No description provided.