Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dpi created an issue. See original summary.

mpp’s picture

gskharmujai’s picture

Version: 8.x-5.x-dev » 8.x-4.3
Assigned: Unassigned » gskharmujai
Status: Active » Needs review
FileSize
3.62 KB

This patch will add a textbox in the CKEditor Linkit plugin settings which will allow to set the minimum length before autocomplete is triggered.

kunalkursija’s picture

@gskharmujai This will be a nice to have feature. Below is some feedback that I thought of while reviewing this patch.

we should not limit size and max length. These can vary based on user requirements and hence should be kept open.

+++ b/src/Plugin/CKEditorPlugin/Linkit.php
@@ -107,9 +110,38 @@ class Linkit extends CKEditorPluginBase implements CKEditorPluginConfigurableInt
+      '#size' => 2,
+      '#maxlength' => 2,

If we go for type 'number', The validation function won't be required.

+++ b/src/Plugin/CKEditorPlugin/Linkit.php
@@ -107,9 +110,38 @@ class Linkit extends CKEditorPluginBase implements CKEditorPluginConfigurableInt
+      '#type' => 'textfield',

We could change the property 'linkit_AutocompleteLimit' to 'linkit_autoCompleteLimit'

+++ b/js/autocomplete.js
@@ -133,6 +133,13 @@
+        autocomplete.options.minLength = selectedTextFormat.editorSettings.linkit_AutocompleteLimit;

One last thing, The latest release for the module is D9 compatible and has got rid of few deprecations. I think it will be better to make this patch available in the 8.x-4.3 version and also reroll it for the latest release.

gskharmujai’s picture

Thanks @kunalkursija. I have made some changes and updated the patch specifically for 8.x-4.3.

Also, i have added the cleanup functionalty to purge the Linkit plugin entries from CKEditor text formats when the Linkit module is uninstalled.

I think i will also need to roll out another patch for 8.x-5.x as there is an implementation change in the CKEditor Linkit plugin.

gskharmujai’s picture

gskharmujai’s picture

gskharmujai’s picture

Looks like i had upload a wrong patch before. This is the correct patch.

3055954-configurable-autocomplete-character-limit-8-v4.3.patch

kunalkursija’s picture

@gskharmujai - i reviewed the patch today and feature seems to be working.

However, I have second thoughts about getting into the text-formats configurations. And on trying few things today I think we could simply create a configuration form at say 'admin/config/content/linkit/autocomplete-minlength' which allows to save integer value for min-length. (default value 1)

We could pass this min-length from processLinkitAutocomplete function of Drupal\linkit\Element\Linkit class to /js/autocomplete.js using drupalSettings. Off-course we will have to define drupalSettings library as a dependency in libraries.yml.

I think this solution will be easier to maintain in the long run.

gskharmujai’s picture

Yes. This implementation is more straightforward and easier to maintain and it removes the dependency on the editor configuration.

I will see how to get this done. I feel that this solution will also work well with 8.x-5.x.

gskharmujai’s picture

gskharmujai’s picture

Uploaded a new patch to fix this issue.

kunalkursija’s picture

I reviewed the patch and here are some suggestions:

+++ b/js/autocomplete.js
@@ -133,6 +133,9 @@
+        var minLength = (settings.linkitMinLength) ? settings.linkitMinLength : 1;
+        autocomplete.options.minLength = minLength;

Remove these and rather let's initialize 'minlength' in the autocomplete object itself. We can do so by adding drupalSettings in the JS file as followed:

  • Replace first line of JS (function ($, Drupal, _, document) { with (function ($, Drupal, _, document, drupalSettings) {
  • Replace last line of JS })(jQuery, Drupal, _, document); with })(jQuery, Drupal, _, document, drupalSettings);
  • The minlength: 1 will become minLength: (drupalSettings.linkitMinLength) ? drupalSettings.linkitMinLength : 1
gskharmujai’s picture

I have updated the patch with these changes. Looks more straightforward and clean now.

kunalkursija’s picture

@gskharmujai The patch worked for me, And I think this looks like a nice to have feature.

This patch might fail and we might need a re-roll to make it work with the latest Linkit release.

gskharmujai’s picture

I had done some ground work to get this patch working for 5.x too a while back where 5.x has some implementation differences.

I think this new patch will work for 8.x-5.x-dev.

kunalkursija’s picture

@gskharmujai the patch application was clean on the latest module release.

gskharmujai’s picture

@kunalkursija : Thank you for spending time to test it out.

gskharmujai’s picture

Re-rolling patch to work for both 6.0.x and 6.1.x.

firewaller’s picture

Updated patch for latest 6.0.x (<=6.0.2)