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

[WIP] candy:core.presence triggerHandler #3

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

malakada
Copy link

@malakada malakada commented Jan 7, 2015

This trigger is used within Candy core here: https://github.com/mojolingo/candy/blob/dev/src/view.js#L69 which really calls this here: https://github.com/mojolingo/candy/blob/dev/src/view/observer.js#L150-L227

No where in that does it call args.from or args.stanza. So uh, I'm not really sure how this is working unless the functionality was fixed/duplicated somewhere else?

I'd like another set of eyes on this to make sure I'm not totally overlooking something.

@malakada malakada changed the title [WIP] Removed unused stanza, and changed 'from' from a JID to user object. [WIP] candy:core.presence triggerHandler Jan 7, 2015
$(Candy).triggerHandler('candy:core.presence', {'from': msg.attr('from'), 'stanza': msg});
$(Candy).triggerHandler('candy:core.presence', {
'from': fromUser
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's probably necessary to include the presence we received here as the state transition.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. This is the one that I was most unsure of since it just simply passes along the entire stanza for other part of the code to pick through.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should wrap that up into nicer accessors for the important bits for sure, but that can go lower priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants