Skip to content
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

clean-up Toaster (#18491) #192

Closed
wants to merge 4 commits into from

Conversation

@dylans
Copy link
Member

commented Sep 12, 2015

This is not complete, it started as an attempt to wrap a topic call in a this.own statement.

Toaster does not appear to implement Destroyable or Evented, so a few things are not working as expected. This patch does a fairly significant best practices clean-up to the widget and test case, but still needs to extend those interfaces to work as expected.

Perhaps @wkeese could take a quick look?

@dylans dylans added this to the 1.11 milestone Sep 12, 2015
@wkeese

This comment has been minimized.

Copy link

commented on widget/Toaster.js in bce6634 Sep 13, 2015

Thanks for all the work on this. Widget#on() is actually meant for external components to listen to events on the widget, rather than to be used internally as this.on(). In the code above, you could just as well use dojo/on or aspect/after instead of this.on(), and in either of those two cases you should technically use this.own() to release the connections when the widget is destroyed. The this.own() thing applies throughout the file, especially for things like the window scroll listener on line 279 (previously 276).

This comment has been minimized.

Copy link

replied Sep 13, 2015

Widget#on() is actually meant for external components to listen to events on the widget, rather than to be used internally as this.on().

PS: Relatedly, the code above doesn't make any sense because the signature of Widget#on() (actually, _WidgetBase#on() is on(type, callback), not on(node, type, callback).

@wkeese

This comment has been minimized.

Copy link
Member

commented Sep 13, 2015

PPS: Rather than using this.connect() or similar, can't you just specify onEnd and onSelect as parameters to the coreFx.slideTo() and coreFx.fadeOut() functions? It seems likely you can, judging from this API doc in dojo/_base/fx.js:

//  |   basefx.animateProperty({
//  |       node: "nodeId",
//  |       // dojo figures out the start value
//  |       properties: { width: { end: 400 } },
//  |       easing: function(n){
//  |           return (n==0) ? 0 : Math.pow(2, 10 * (n - 1));
//  |       },
//  |       onEnd: function(node){
//  |           // called when the animation finishes. The animation
//  |           // target is passed to this function
//  |       }
//  |   }).play(500); // delay playing half a second

In any case, you shouldn't worry about leaks from advice on non-DOM methods like an animation's onEnd method. If anything, you should worry about canceling running animations when the widget is destroyed.

@dylans

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2015

Thanks @wkeese clearly I was getting fatigued by the time I got to this one.

I've updated the pull request as follows:

  • Switch to using the effects event syntax for onEnd of effects, and wrap handlers with calls to hitch
  • Switch the select call to aspect.after as it made more sense given the default onSelect handler

That said, this.own does not appear to be defined on the Toaster, so I think something is wrong with the inheritance chain here. Any ideas on why Toaster doesn't seem to have inherited from dijit/Destroyable?

@@ -76,10 +78,11 @@ define([
this.hide();

// place node as a child of body for positioning
this.ownerDocument.body.appendChild(this.domNode);
this.domNode.ownerDocument.body.appendChild(this.domNode);

This comment has been minimized.

Copy link
@wkeese

wkeese Sep 14, 2015

Member

Actually _WidgetBase has an ownerDocument property so you don't need this change.

This comment has been minimized.

Copy link
@dylans

dylans Sep 14, 2015

Author Member

More evidence that something is wrong with the inheritance chain here, as this.ownerDocument is not defined, which is why I made the switch to at least get it working again...


if(this.messageTopic){
connect.subscribe(this.messageTopic, this, "_handleMessage");
// TODO: wrap in a call to this.own
topic.subscribe(this.messageTopic, lang.hitch(this, "_handleMessage"));

This comment has been minimized.

Copy link
@wkeese

wkeese Sep 14, 2015

Member

this.subscribe() is probably best, or second best this.own(topic.subscribe(...)). And in either case removing the TODO comment.

This comment has been minimized.

Copy link
@dylans

dylans Sep 14, 2015

Author Member

I tried this.own, but it's not defined.

this._placeClip();

if(!this._scrollConnected){
this._scrollConnected = connect.connect(window, "onscroll", this, this._placeClip);
// this._scrollConnected = this.own(on(window, "scroll", lang.hitch(this, "_placeClip")));
// this._scrollConnected = on(window, "scroll", lang.hitch(this, "_placeClip"));

This comment has been minimized.

Copy link
@wkeese

wkeese Sep 14, 2015

Member

I guess there's still work to be done here. Second one seems good.

This comment has been minimized.

Copy link
@dylans

dylans Sep 14, 2015

Author Member

Yes, I'm trying to figure out why this.own isn't working first. :)

@wkeese

This comment has been minimized.

Copy link
Member

commented Sep 14, 2015

Any ideas on why Toaster doesn't seem to have inherited from dijit/Destroyable?

Hmm, it must, since it inherits from _WidgetBase, and _WidgetBase inherits from Destroyable. Probably at the point in the code this was not pointing to the Toaster instance?

@dylans

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2015

Hmm, it must, since it inherits from _WidgetBase, and _WidgetBase inherits from Destroyable. Probably at the point in the code this was not pointing to the Toaster instance?

That's what I would think as well, but something isn't right here. I also used registry to get a reference to the widget instance, and own is not defined on the Toaster instance.

@dylans

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2015

Cleaned-up, works in Chrome and FF, if someone has time to test in IE8-11 that would be great, otherwise I'll get to that soon.

@dylans

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2015

Passes quick testing on desktop browsers ( FF latest, Chrome latest, Safari latest, IE8-11, MS Edge). I'll land this unless I hear other suggestions in the next day or so.

@@ -79,7 +81,8 @@ define([
this.ownerDocument.body.appendChild(this.domNode);

if(this.messageTopic){
connect.subscribe(this.messageTopic, this, "_handleMessage");
// TODO: wrap in a call to this.own
this.own(topic.subscribe(this.messageTopic, lang.hitch(this, "_handleMessage")));

This comment has been minimized.

Copy link
@wkeese

wkeese Sep 14, 2015

Member

As I said before, you can get rid of the TODO comment here, and also it's slightly better to just call this.subscribe() instead of this.own(topic.subscribe(...

This comment has been minimized.

Copy link
@dylans

dylans Sep 14, 2015

Author Member

Yes, missed it, thanks. Part of me feels like not using this.subscribe as it feels wrong that it's there, but that's perhaps silly. :)

this._setHideTimer(duration);
this.connect(this, 'onSelect', function(){
this.own(aspect.after(this, 'onSelect', lang.hitch(this, function(){

This comment has been minimized.

Copy link
@wkeese

wkeese Sep 14, 2015

Member

Alternately (and more tersely) you could use this.on("select", ...)

This comment has been minimized.

Copy link
@dylans

dylans Sep 14, 2015

Author Member

Tried that earlier and was getting Error: Target must be an event emitter notifications, will try again.

<button type="submit" id="submitButton2"
>Click to show another message</button>
<button type="submit" id="submitButton3"
>Click to show yet another message</button>

This comment has been minimized.

Copy link
@wkeese

wkeese Sep 14, 2015

Member

Not sure why you changed the click handlers to be set up programatically instead of declaratively... I guess because the variables like showTestMessage aren't global anymore?

This comment has been minimized.

Copy link
@dylans

dylans Sep 14, 2015

Author Member

Exactly, because they're not globals.

toast = dijit.byId("toast");
on(dom.byId("submitButton1"), "click", showTestMessage);
on(dom.byId("submitButton2"), "click", showAnotherMessage);
on(dom.byId("submitButton3"), "click", showYetAnotherMessage);

This comment has been minimized.

Copy link
@wkeese

wkeese Sep 14, 2015

Member

No need for showToastMessage etc. variables, the function definitions can be inlined here.

@dylans

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2015

Thanks for the feedback, addressed.

@dylans

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2015

Closed via 56ee55d. Thanks for the feedback.

@dylans dylans closed this Sep 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.