ckeditor4-plugin-a11ychecker icon indicating copy to clipboard operation
ckeditor4-plugin-a11ychecker copied to clipboard

T/204 provide custom Quail path.

Open Tade0 opened this issue 9 years ago • 1 comments

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.

Tade0 avatar Jun 27 '16 10:06 Tade0

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.

f1ames avatar Aug 03 '16 08:08 f1ames