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

added some fixes from @tarr11 to codemirror integration #119

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

Conversation

automata
Copy link

added suggested fixes from @tarr11: https://github.com/tarr11/codeshare/

the old webclient/cm.js didn't works properly. this one is working the same way the ace backend (at least in my tests).

I also changed examples/cm/index.html. so we can test this on the bin/exampleserver and then http://localhost:8000/cm

fix-cm branch.

thanks about your incredible work on sharejs!

@travisbot
Copy link

This pull request passes (merged cf7cc46 into c366f99).

@wmertens
Copy link
Contributor

One more thing, you just recompiled the webclient right (cake webclient)? Or did you make changes to it? The webclient is compiled from src/client/ and any changes you made should be made there...

@wmertens
Copy link
Contributor

@automata So can you add the changes you made to https://github.com/josephg/ShareJS/blob/master/src/client/cm.coffee ? This pull request only has cm.js which gets overwritten every time someone runs cake webclient

editor.setOption('onChange', null);
return delete this.detach_cm;
doc.detach_codemirror = function () {
editorDoc.removeListener('change', editorListener);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be editorDoc.setOption('onChange', null) for codemirror 2+

@wmertens
Copy link
Contributor

@automata I would love to merge this but in its current state I can't. Any chance you could change the pull request like discussed in the comments?

Thanks!

@wmertens
Copy link
Contributor

@automata ping?

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

Successfully merging this pull request may close these issues.

None yet

4 participants