Ckeditor5: imageInsert and imageUpload commands should also accept singular params?

Created on 19 Nov 2018  Â·  4Comments  Â·  Source: ckeditor/ckeditor5

It's just so weird and unclear:

editor.execute( 'imageInsert', { sources: urls } );

Did you think that it inserts a single image with multiple sources (i.e. a responsive image). Cause I did.

Initially imageUpload command accepted just a single file, but for some reason we needed it to accept multiple files. Now, the imageInsert command follows that... and it's just confusing. What could help is if:

  • the param name was singular and just allow passing arrays,
  • support singular and plural forms at the same time.
image discussion improvement

Most helpful comment

AFAIR we already have some rules for options which can be singular or plural.

The rule is, start with a singular, add support for plural (which often happens later), but don't make it plural then. That's for param names. Example – DowncastWriter#setStyle(). It's different for object definitions of some types (ViewElementDefinition) when the plural is often the default.

So, I think that both those commands should accept a param being a singular name (file, source). But this param should also accept an array.

All 4 comments

cc @jodator @pjasiun @oskarwrobel

Support for uploading multiple images in one command was introduced when we wanted to disable image upload when the image is selected. When we used the command for single images we were not able to do it because after the first image was uploaded the selection was moved on that image so you were not able to upload the next image.

  • the param name was singular and just allow passing arrays,
  • support singular and plural forms at the same time.

I am fine with both options. We should have some global rules for such parameters and just follow them. AFAIR we already have some rules for options which can be singular or plural.

So to referesh: The image upload (which was the only method to insert images back then) was changed to support multiple images due to a bug which disallowed to insert multiple images. Here is the long commented PR: https://github.com/ckeditor/ckeditor5-image/pull/228.

The sources name might be unfortunate though.. so I'm OK with changing it in the PR.

AFAIR we already have some rules for options which can be singular or plural.

The rule is, start with a singular, add support for plural (which often happens later), but don't make it plural then. That's for param names. Example – DowncastWriter#setStyle(). It's different for object definitions of some types (ViewElementDefinition) when the plural is often the default.

So, I think that both those commands should accept a param being a singular name (file, source). But this param should also accept an array.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

jodator picture jodator  Â·  3Comments

pjasiun picture pjasiun  Â·  3Comments

devaptas picture devaptas  Â·  3Comments

oleq picture oleq  Â·  3Comments

hybridpicker picture hybridpicker  Â·  3Comments