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

Rewrite the jQuery parts to not require this library #7

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@linkmauve
Contributor

linkmauve commented Nov 14, 2018

jQuery is a quite heavy library for the very few places it was used in this library.

@linkmauve linkmauve force-pushed the linkmauve:no-jquery branch 2 times, most recently from dc8ae18 to 904fd2e Nov 29, 2018

Rewrite the jQuery parts to not require this library
jQuery is a quite heavy library for the very few places it was used in
this library.

@linkmauve linkmauve force-pushed the linkmauve:no-jquery branch from 904fd2e to def2c30 Nov 30, 2018

@linkmauve

This comment has been minimized.

Contributor

linkmauve commented Nov 30, 2018

This is now rebased on master.

@sualko

The CustomEvent would be a breaking change, so we have to make a major bump, or stay backward compatible.

});
});
self.manager.on('incoming', function(session) {
session.on('change:connectionState', function(session, state) {
$(document).trigger('iceconnectionstatechange.jingle', [session.sid, session, state]);
var evt = new CustomEvent('iceconnectionstatechange.jingle', [session.sid, session, state]);

This comment has been minimized.

@sualko

sualko Nov 30, 2018

Member

According to MDN the second parameter has to be a dictionary with a key detail.

It's not compatible with Safari, but I think this is ok, because they not even support WebRTC. Maybe we should handle this case more gracefully.

Anyway this would be a breaking change, because it's not compatible with the previous jquery. Maybe we can use this as fallback if jquery is not available. What do you think?

@@ -116,7 +120,7 @@ var IqStanza = jxt.getDefinition('iq', 'jabber:client');
iq.id = self.connection.getUniqueId('sendIQ');
}
self.connection.send($.parseXML(iq.toString()).getElementsByTagName('iq')[0]);
self.connection.send((new DOMParser()).parseFromString(iq.toString(), 'text/xml').getElementsByTagName('iq')[0]);

This comment has been minimized.

@sualko

sualko Nov 30, 2018

Member

Maybe we should split this line to make it more readable

@sualko

This comment has been minimized.

Member

sualko commented Nov 30, 2018

I want to thank you that you worked on this at the Dusseldorf XMPP sprint. I'm quite upset that I was not able to join the event, but maybe next time.

Btw I added you as a member to the repository, because as I read you are trying to include this into conversation and therefore you probably have to make even more changes. It would be nice if you could still open pr, so that I get informed what is going on here. Happy coding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment