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

T/204 provide custom Quail path. #218

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

T/204 provide custom Quail path. #218

wants to merge 5 commits into from

Conversation

Tade0
Copy link
Contributor

@Tade0 Tade0 commented Jun 27, 2016

Fixes #204.

Making a manual test here was not much of an option since normally quailInclude.js is present only in the built version.

With some modifications though it was possible to create a unit test which checks whether an error is thrown in case the loading fails.

@f1ames
Copy link
Contributor

f1ames commented Aug 3, 2016

Overall the code looks good but it seems to me that there are a few things to polish:

  1. The test which checks script loading was added but there is no test which checks if CKEDITOR.config.a11ychecker_quailPath works correctly (which is the aim of this ticket/PR as I understand correctly). Maybe it is possible to create such test using the mock similar to the one in TC mentioned above?
    I assume that "since normally quailInclude.js is present only in the built version" also may cause some troubles here.
  2. I guess some documentation/description should be added to CKEDITOR.config.a11ychecker_quailPath.
  3. I assume this line
    var quailPath = 'plugins/a11ychecker/libs/quail/quail.jquery.min.js' || CKEDITOR.config.a11ychecker_quailPath;
    should be the other way around
    var quailPath = CKEDITOR.config.a11ychecker_quailPath || 'plugins/a11ychecker/libs/quail/quail.jquery.min.js').
    In the current state, first value is always set (so the second is never used) and the minifier (while building) changes it to
    var a = 'plugins/a11ychecker/libs/quail/quail.jquery.min.js';
    which does not make sense in this case.

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.

None yet

2 participants