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

add spellchecker extension #530

Merged
merged 5 commits into from
Mar 17, 2016
Merged

Conversation

jcb91
Copy link
Member

@jcb91 jcb91 commented Mar 11, 2016

No description provided.

@jcb91 jcb91 mentioned this pull request Mar 11, 2016
@jcb91 jcb91 force-pushed the feature/spellchecker branch from 87af3cd to 4d1f2aa Compare March 11, 2016 14:05
@jankatins
Copy link
Member

Yay!

[NextStepWebs/codemirror-spell-checker](https://github.com/NextStepWebs/codemirror-spell-checker/blob/78773ebdd6c8cf8acd043342023636ae345ca0f3/src/js/spell-checker.js)
at the
[suggestion](https://github.com/ipython-contrib/IPython-notebook-extensions/issues/521)
of [@JanSchulz](https://github.com/JanSchulz).
Copy link
Member

Choose a reason for hiding this comment

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

Thanks but IMO there is no need for that (-> s/at the .*//). :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, you're welcome. Besides, I like to keep a history of where ideas come from, sometimes it's handy to remind yourself when you want to come back to something after not looking at it for some time!

@jankatins
Copy link
Member

Tried it, it works: unfortunately no replacement suggestions :-(

@jcb91 Have you tried the other way with codemirror type "contenteditable" and then add a spellcheck=true to each span?

-> codemirror/codemirror5#1017 (comment)

@jcb91
Copy link
Member Author

jcb91 commented Mar 13, 2016

Yeah, no replacement suggestions as yet, although since Typo.js provides suggestions, I expect they could be added as a popover or something fairly easily. I didn't bother trying the contenteditable method, since as huangp notes in codemirror/codemirror5#1017 (comment), it doesn't trigger correct events for CodeMirror to update the text when replacements are used, which could lead to a whole bunch of weird inconsistent-state problems. Maybe I'll give it a try and see how horrible it is, though 😛

@jankatins
Copy link
Member

re contenedible: from the above issue, I got the feeling that in 2014 they didn't want to include a contentedible part ("Okay, yeah, that uses contentEditable, which is not going to happen in CodeMirror.") but the documentation on this was included in 2015 and includes the wording

the "contenteditable" model. The intention is to make it the default on modern desktop browsers in the future.

But searching for spellcheckin the codemirror codebase, I found this: https://github.com/codemirror/CodeMirror/blob/877b2b1766a40bed35d5f035f15e01d8f9a66e4f/lib/codemirror.js#L1574

-> It seems that they disable spellcheck even if using contentedible, so maybe using contentedible and then adding the spellcheck attribute back would be already enough?

@jcb91
Copy link
Member Author

jcb91 commented Mar 13, 2016

True, they do seem to use contentEditable at least on mobile, so it may be worth a shot. The fact that they have explicitly disabled spellcheck makes me suspicious that the events won't cooperate, but whatever, we can give it a try, at least...

@jankatins
Copy link
Member

Re disable spellcheck: this is also used on the "other version" and there it is used to disable the spellcheck on the (hidden?) textarea... So another explanation could be that they disabled it in the "regular" version and just used the same thing in the to-be-new version...

…ionaries, and a script to do it relatively painlessly
@jcb91
Copy link
Member Author

jcb91 commented Mar 13, 2016

OK, I've added a more complete explanation to the readme, including some python sample code, and actually tried switching to other dictionaries. It seems to work ok for me.

You've lost me a bit with the "other version" - I'm not quite sure what you mean?

@jcb91
Copy link
Member Author

jcb91 commented Mar 13, 2016

Unfortunately, it seems that the "contenteditable" model referred to in the docs doesn't actually set the contentEditable attribute at all... , which is exactly what you were saying. Apologies, I'm being a bit slow!

@jankatins
Copy link
Member

Sorry: "other version" = whatever is not the "contenteditable" mode.

Actually I really thought, that setting the "contenteditable" mode in codemirror would use something in the backend which sets "contenteditable to the used html element. So it seems that I misled you :-( Sorry again!

@jcb91
Copy link
Member Author

jcb91 commented Mar 14, 2016

A few fixes, the most prominent being that I hadn't got raw downloads of the dictionaries - I managed to get round this by using the base64-encoded versions which are provided.

@jankatins
Copy link
Member

Just to let you know: I'm using this now for a few days and I'm loving it. It would be perfect with suggestions, but this is great as it is :-)

@jcb91
Copy link
Member Author

jcb91 commented Mar 17, 2016

okey doke, I'll merge as-is then, and maybe add suggestions later on...

jcb91 added a commit that referenced this pull request Mar 17, 2016
@jcb91 jcb91 merged commit 3333547 into ipython-contrib:master Mar 17, 2016
@jcb91 jcb91 deleted the feature/spellchecker branch March 17, 2016 12:36
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