-
Notifications
You must be signed in to change notification settings - Fork 65
Toastr service and component #1614
Toastr service and component #1614
Conversation
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: a49ebf3 (Please note that this is a fully automated comment.) |
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 28c09b6 (Please note that this is a fully automated comment.) |
src/modules/toast/toast.service.ts
Outdated
import { BehaviorSubject } from 'rxjs/BehaviorSubject'; | ||
import { Observable } from 'rxjs/Observable'; | ||
|
||
export enum ToastType { |
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.
All custom classes and types should be prefixed with Sky
.
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.
Done
src/modules/toast/toast.service.ts
Outdated
|
||
constructor(public message: string, public toastType: string, private removeFromQueue: Function, timeout?: number) { | ||
if (timeout) { | ||
this.timeout = setTimeout(this.close, timeout); |
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.
@Blackbaud-ToddRoberts @Blackbaud-MattGregg I'm thinking that we don't want to automatically close toast messages (after a certain timeout) for accessibility reasons. I read somewhere that an alert shouldn't automatically dismiss itself, but should be a deliberate action, to allow screen readers adequate time to read out the message?
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.
@Blackbaud-SteveBrush yes, you are correct. For time limits that are set on content/pages, there needs to be a way for users to turn off or extend the time it's visible so someone can read it.
See https://www.w3.org/TR/UNDERSTANDING-WCAG20/time-limits-required-behaviors.html. The other way to handle this would be like you said to just not have the time limit.
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.
The timeout toast in UX1 extends the duration if the mouse is hovered over it. Are there alternative approaches we could look at to maintain the functionality while being accessible? Are there techniques to make content visible to screen readers but not in the UI, such that we could maintain them in the background for screenreaders keep reading?
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.
Hey Matt, thank you for weighing in. For now we're going to avoid including the timout functionality and will consider adding it in a later version.
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.
Todd, I could recreate that functionality. Add an "extendedTimeout" duration that defaults to something and will extend the timeout when hovered over. I've also been considering disabling the timeout on hover/focus/
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.
@Blackbaud-ToddRoberts the success criteria isn't specific for users of screenreaders. It's really meant for a range of people who might need more time to complete tasks or read content for a variety of reasons.
src/modules/toast/toast.service.ts
Outdated
this._isClosed.next(true); | ||
} | ||
} | ||
export interface ToastConfig { |
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.
Every export should be its own 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.
done
@@ -0,0 +1,19 @@ | |||
<div class="sky-toaster"> |
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.
This is a great start, but we'll need to inject the Toast "host" component at the bottom of the page, just before the closing <body>
tag, to make sure that they always appear outside the normal flow of the document. See how we did it with the Flyout component, as an example: https://github.com/blackbaud/skyux2/blob/master/src/modules/flyout/flyout.service.ts#L54-L65
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.
done
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.
This doesn't seem to incorporate any assistive technology support from what I can see. Especially for the case where type=danger for error identification, but I think we should support this for all the type. This could be done an ARIA live region. It needs to be present in the DOM empty and then have content injected. See - https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Live_Regions
For type=danger it would make sense to use aria-live="assertive" so this is read out immediately. While the other types would be aria-live="polite" which is more the intent for these to be not immediate/interruptive.
Another decent reference - https://www.w3.org/TR/WCAG20-TECHS/ARIA19.html
IBM's Carbon Design system notification component is a good example.
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.
@Blackbaud-MattGregg
Alright, I've added an "alert" role to the toast container and aria-live attributes to the toasts based on if they are "danger" or not (polite versus assertive). It looks correct when I run it in NVDA, but is this sufficient or are there other concerns?
Sorry, I'm still not fully familiar with accessibility requirements, but I'm going through the learning videos right now.
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.
@blackbaud-conorwright that's great. If those things are in, it should be good to go from this accessibility perspective. thanks.
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.
Sweet! Thank you 😄
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 3b5d5cb (Please note that this is a fully automated comment.) |
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 35de170 (Please note that this is a fully automated comment.) |
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: ce8ba62 (Please note that this is a fully automated comment.) |
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: bc37187 (Please note that this is a fully automated comment.) |
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: ff5093c (Please note that this is a fully automated comment.) |
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 83e7d04 (Please note that this is a fully automated comment.) |
@blackbaud-sky-savage retry |
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 83e7d04 (Please note that this is a fully automated comment.) |
@blackbaud-sky-savage retry |
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 83e7d04 (Please note that this is a fully automated comment.) |
[attr.aria-live]="message.toastType==='danger' ? 'assertive' : 'polite'"> | ||
<div | ||
class="sky-toast-content" | ||
(click)="message.close()"> |
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.
We probably don't want to close the toast when its content is clicked, as this will cause bubbling issues if the template includes a link or some form of click interaction. See IBM's notification component as a good example: http://carbondesignsystem.com/components/notification/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.
removed. Just the close button now
@@ -0,0 +1 @@ | |||
<b class="sky-custom-toast">Toast component</b> |
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.
Prefer strong
over b
. https://davidzych.com/why-use-strong-and-em-over-b-and-i/
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.
done
@@ -0,0 +1,15 @@ | |||
import { Component } from '@angular/core'; |
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.
Needs barrel import format.
@@ -0,0 +1,22 @@ | |||
import { Component } from '@angular/core'; | |||
import { SkyToastService } from '@blackbaud/skyux/dist/core'; |
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.
Needs barrel import format.
@@ -1017,6 +1018,22 @@ export class SkyDemoService { | |||
} | |||
] | |||
}, | |||
{ | |||
name: 'Toast', |
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.
Are you missing the "custom" toast component here?
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.
added
public message: string, | ||
public customComponentType: Type<any>, | ||
public toastType: string, | ||
private removeFromQueue: Function, |
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.
Private properties should be listed after public.
#1647 (comment)
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.
done
if (!this._isClosing.getValue()) { | ||
this._isClosing.next(true); | ||
|
||
setTimeout(() => { |
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.
Possible to remove this setTimeout
somehow? If not, we'll need to figure out how to use the SkyWindowRefService
...
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.
Well this is to achieve the fade effect for toast messages. So when you close the toast, it fades out in css and then is actually closed. An alternate would be to just not include the fade at all 😛
Then the isClosing state would also no longer be necessary
Otherwise, I'm not sure of a good alternate that let's it wait for a certain duration while the css animation completes.
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.
Got it with the animations set up. Switched the setTimeout to be a subscribe to the closed event
private _isClosed: BehaviorSubject<boolean>; | ||
private _isClosing: BehaviorSubject<boolean>; | ||
|
||
public get isClosed(): Observable<boolean> { return this._isClosed.asObservable(); } |
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.
Curious if we should rewrite these to be events? closed
could be a one-time fired EventEmitter
.
#1647 (comment)
For isClosing
, since you're relying on a programmatic method to trigger the fade-out animation, perhaps it would be better to use Angular's animations
features in the SkyToastComponent
class itself, instead of CSS transitions. The flyout component is doing the same thing, if you need an example of the behavior.
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.
Oh, I wasn't aware that existed. I'll look into the angular animations and take a look at flyout. Closed can definitely be an emitter
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.
done
@@ -0,0 +1,8 @@ | |||
<div | |||
role="alert" | |||
class="sky-toaster"> |
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.
The CSS class name for the component should mirror the component selector. I personally prefer sky-toaster
; perhaps that could be the name of the component selector too?
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.
I love sky-toaster. I'll rename the container ;)
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.
Apologies for drive-by comment...only watching this because team is looking forward to some toast 😄 Thanks for bringing this component to SKY UX 2!
But would this mean that usages of this component will use "toaster", e.g., the ToasterService
or just the container? Isn't Toast
the more common terminology for this sort of component? Or maybe the ToastService creates "toast" but it seems more uniform the component itself should be toast
? Just something to consider, as we'll be using either way 😉
E.g.,
https://material.angularjs.org/latest/demo/toast,
https://material.io/guidelines/components/snackbars-toasts.html#, react https://tomchentw.github.io/react-toastr/ (library is called toastr but the component itself is toast)
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.
Hey @Blackbaud-IsaacAggrey , I believe that there may be some confusion here. This file is the template for the toast-container component. The actual toast component will be called sky-toast
.
This component is just a container that is injected into the view to contain any Toasts created by the ToasterService.
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.
Gotcha, thanks for clearing that up for me @blackbaud-conorwright . 😄
return this.open(config); | ||
} | ||
|
||
public openTemplatedMessage(customComponentType: Type<SkyToastCustomComponent>, config: SkyToastConfig = {}, providers?: Provider[]) { |
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.
I would avoid extending components, since they can include lots of things the consumer is unaware of. Angular's philosophy seems to indicate that new features should be injected into the component's constructor.
So, instead of requiring your consumers to extend SkyToastCustomComponent
, they could instead expect to have a dependency injected into their constructor with everything they need to interact with the toast. For example:
my-toast.component.ts (something the consumer writes)
export class MyToastComponent {
constructor(
private instance: SkyToastInstance
) {
// Set any values from instance.config...
}
public ngOnInit() {
this.instance.closed.subscribe(() => {
// Do something when the toast instance is closed...
});
}
public close() {
// Allow the custom template to close the toast:
this.instance.close();
}
}
toast-instance.ts (a class written by SKY UX)
export class SkyToastInstance {
public config: SkyToastConfig;
public closed = new EventEmitter();
public close() {
this.closed.emit();
this.closed.complete();
}
}
toast.component.ts
private loadComponent() {
// ...
const instance = new SkyToastInstance();
instance.config = config;
this.providers.push({
provide: SkyToastInstance,
useValue: instance
});
// ...
}
This code is untested, but we're doing something quite similar in the modal component. Definitely happy to talk about this one more if needed.
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.
done. I renamed the message entity to Instance and switched to injection without the custom interface
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 01c38d6 (Please note that this is a fully automated comment.) |
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 2fa1e41 (Please note that this is a fully automated comment.) |
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 57bfe9c (Please note that this is a fully automated comment.) |
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.
Thanks for taking another look! These comments deal mostly with implementation, and finalizing the public component/service API.
@Component({ | ||
selector: 'sky-test-cmp-toast', | ||
templateUrl: './toast-demo.component.html', | ||
providers: [SkyToastService], |
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.
I don't think this providers array is needed.
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.
gone
|
||
let toastType: string = 'info'; | ||
switch (config.toastType) { | ||
case SkyToastType.Success: |
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.
I see you’re translating the SkyToastType
to a string value for the class names. Is there an instance where an enum
is better than a custom string type? For example:
export type SkyToastType = 'success' | 'warning' | 'danger' | 'info';
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.
switched
|
||
import { | ||
SkyToastContainerComponent | ||
} from '../toast-container.component'; |
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.
Remind me, were we going to rename toast-container
to toaster
(SkyToasterComponent
)? If so, the file should probably be renamed, as well as the TypeScript class name.
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.
changed to toaster
return this.open(config); | ||
} | ||
|
||
public openTemplatedMessage(customComponentType: Type<any>, config: SkyToastConfig = {}, providers?: Provider[]): SkyToastInstance { |
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.
When I'm writing a public API for a service, I like to put a lot of thought around how it will be documented. In this case, we'd be documenting three different methods, all of which would be using the same config object, which becomes slightly problematic:
For one, a user could use openMessage('my message', { message: 'diff message' })
and it wouldn't be immediately apparent which "message" would be used (same with openTemplatedMessage
). Also, if this method is used, the customComponentType
option becomes useless.
Another scenario: the openTemplatedMessage()
method also uses the same config object, but the message
option is useless. Thinking about how to document this, it becomes confusing: "If you want to open a toast with a custom component, don't worry about setting the message
property in the config because it doesn't do anything."
In this case, it would probably be a good idea to remove message
and customComponentType
from the config altogether and add them as first-class arguments to their respective methods, leaving us with only two public methods on the service.
toast-config.ts
export interface SkyToastConfig {
providers?: Provider[];
type?: SkyToastType;
}
toast.service.ts
public openMessage(message: string, config?: SkyToastConfig): SkyToastInstance<any> {
const context = new SkyToastBodyContext();
context.message = message;
config.providers = [{
provide: SkyToastBodyContext,
useValue: context
}];
return this.openComponent(SkyToastBodyComponent, config);
}
public openComponent<T>(component: Type<T>, config?: SkyToastConfig): SkyToastInstance<T> {
return this.open(component, config);
}
Both public methods use the same private method:
private open<T>(component: Type<T>, config?: SkyToastConfig): SkyToastInstance<T> {
if (!this.toaster) {
this.toaster = this.createToaster();
}
const toast = new SkyToastInstance(component, config);
this.toaster.instance.addToast(toast);
this.notifyNewToastInstance(toast);
return toast;
}
toast-body.component.ts
@Component({
selector: 'sky-toast-body',
templateUrl: './toast-body.component.html'
})
export class SkyToastBodyComponent {
constructor(
public context: SkyToastBodyContext
) { }
}
toast-body.component.html
<div class="sky-toast-body">
{{ context.message }}
</div>
In this way, both public methods would use the same config, as well as the same implementation for injecting the component into the toast template. (Another good side effect of this setup: you won't need the error messages in the private createMessage()
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.
Alright, yeah I like having those separate. I think I was originally maintaining all three for parity with skyux 1, but we really don't need all three.
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.
switched to be two specific methods and 3 less properties on the config
private host: ComponentRef<SkyToastContainerComponent>; | ||
|
||
private _messages: SkyToastInstance[] = []; | ||
public messages: BehaviorSubject<SkyToastInstance[]> = new BehaviorSubject([]); |
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.
Can we rename this public property messages
to something less ambiguous (not to be confused with the message
property below)? toastInstances
might work...
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.
Oh yeah, I should go rename all my variable names for those now that it's SkyToastInstance anyway
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.
done
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.
changed to toastInstances
}) | ||
export class SkyToastComponent implements OnInit, OnDestroy { | ||
@Input('message') | ||
public message: SkyToastInstance; |
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.
Mind if we rename this to instance
to not be confused with the "message" property of the default toast context?
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.
changed
public message: SkyToastInstance; | ||
|
||
@ViewChild('skytoastcustomtemplate', { read: ViewContainerRef }) | ||
private customToastHost: ViewContainerRef; |
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.
Since you'll be injecting a component in both the default and "custom" implementations, you can rename this to something more generic, like toastHost
or toastTarget
.
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.
toastHost it is
) {} | ||
|
||
public getAnimationState(): string { | ||
return !this.message.isClosing.isStopped ? TOAST_OPEN_STATE : TOAST_CLOSED_STATE; |
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.
You probably don't need to wait for isClosing
to fire to start the animation since the toast will be "open" when it's first created. Something like the following should be enough:
private isOpen = true;
public getAnimationState(): string {
return (this.isOpen) ? TOAST_OPEN_STATE : TOAST_CLOSED_STATE;
}
public onAnimationDone(event: AnimationEvent) {
if (event.toState === TOAST_CLOSED_STATE) {
this.instance.close();
}
}
// Bind to the "x" button click:
public close() {
this.isOpen = false;
this.changeDetector.markForCheck();
}
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.
switched to isOpen
animations: [ | ||
trigger('toastState', [ | ||
state(TOAST_OPEN_STATE, style({ opacity: 1 })), | ||
state(TOAST_CLOSED_STATE, style({ opacity: 0 })), |
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.
I might need to be reminded of the requirements, but if you want the toast to fade in (and not just appear), you can add:
transition('void => *', [
style({ opacity: 0 }),
animate('250ms linear')
]),
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.
I only really want the fade out effect. I've switched this to be toastOpen => toastClosed
public customComponentType: Type<any>, | ||
public toastType: string, | ||
public providers: Provider[] = [], | ||
private removeFromQueue: Function |
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.
Is there a way we can remove this removeFromQueue
method from the instance? It feels like a "bleeding scope" because we don't know what a "queue" is in this context (or who it belongs to).
Maybe the owner of the removeFromQueue
method could observe this instance's closed
property?
export class SkyToastInstance<T> {
public closed = new EventEmitter<void>();
constructor(
public componentInstance: Type<T>,
public config: SkyToastConfig
) { }
public close() {
this.closed.emit();
this.closed.complete();
}
}
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.
Oh! That's perfect! I had so many issues with the design around passing removeFromQueue to the toast. Subscribing to its closed is perfect.
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.
switched to subscribing on creation in the service (to remove from queue)
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 6e766de (Please note that this is a fully automated comment.) |
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 15103e0 (Please note that this is a fully automated comment.) |
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: ac1e7bd (Please note that this is a fully automated comment.) |
@blackbaud-conorwright Closing this in favor of: #1676 (in order to run the full suite of tests). Thanks for all your hard work on this! |
This is the from scratch approach. If it doesn't turn out well, we may go back to using an external library.
This is a work in progress. Do not merge this branch until I declare it complete.This work is ready for review