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

SWIK 1872 Inline spell checker (e.g. browser spell checker) needed when editing slides #947

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

kadevgraaf
Copy link
Member

No description provided.

@kadevgraaf
Copy link
Member Author

I have created a PR which enables the native browser spell-check, which is probably better than a CKeditor plugin (e.g. https://ckeditor.com/cke4/addon/scayt which relies on a webservice) because browsers support many language, easily add these, and have a dictionary.

One downside is that we have the context menu enabled, and someone thus has to press the Ctrl or Meta (Mac) key when right-clicking to show the native browser context menu which has the spell checker (I have added help menu instructions for this). This whereas scayt would be displayed directly in the context menu. Misspelled words are highlighted correctly (red underlining) with my changes.

Removing the entire context menu might also be an option - it now shows "copy/paste/etc." with most elements, which are also covered by native browser context menu, and only for tables it shows a really different menu, which is functionality we would then lose.. however, I could make a workaround for this or https://ckeditor.com/cke4/addon/tabletoolstoolbar

Another option; making the CKeditor context menu appear on pressing ctrl/meta key is not easily possible for some reason (see e.g. https://ckeditor.com/cke4/addon/contextmenu with unanswered questions)

@abijames
Copy link
Contributor

Tested branch with CKeditor context menu removed. This worked well for text (copy/paste provided by bowser context menu).

For images and embedded content I could double click to get to properties but this is difficult to discover and not accessible. There was no way to access properties on tables or equations. Tables addon would be a nice solution but need to test equations further as this could be caused by the bug of equations not showing in slide edit

Tested on http://localhost:3000/deck/4377/_/slide/36389-4/36389-4:1/edit

…wser-spell-checker)-needed-when-editing-slides
…wser-spell-checker)-needed-when-editing-slides
@kadevgraaf
Copy link
Member Author

For Images there is now the edit image context menu item (in the top-left context-menu) by Luis.
I could make something similar for embedded content (altough the only thing relevant to change seems the title (which is only used by screenreaders I guess) and URL) - this is also discussed in #980.

@kadevgraaf
Copy link
Member Author

The https://ckeditor.com/cke4/addon/tabletoolstoolbar plugin does not work (it needs the context menu to function).
Also re-adding the table button on the main CKeditor menu only allows someone to create tables, not edit.
What I could do is detect if there is a table in slide content, and then enable the context menu to allow table editing. Would that be a usable solution? The context menu would change between native browser and ckeditor context menu, which is inconsistent and might confuse users...

Reintroducing the Ckeditor context menu would still provide native browser spellchecking in the sense that misspelled words are highlighted with red, but users cannot see suggestions for corrections unless pressing control+click (to show native browser context menu).

@abijames
Copy link
Contributor

@kadevgraaf do we still want to try to get this into the platform?

…wser-spell-checker)-needed-when-editing-slides
@kadevgraaf
Copy link
Member Author

@kadevgraaf do we still want to try to get this into the platform?

I think it has a high risk for a change that is not visible + we could improve it a bit more probably, so lets go for next release.
Sorry for my late reply.

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

Successfully merging this pull request may close these issues.

2 participants