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

How and when should this feature warn about missing upload adapter? #2824

Closed
Reinmar opened this issue Sep 29, 2017 · 3 comments
Closed

How and when should this feature warn about missing upload adapter? #2824

Reinmar opened this issue Sep 29, 2017 · 3 comments
Labels
package:upload status:discussion type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@Reinmar
Copy link
Member

Reinmar commented Sep 29, 2017

Followup of #569.

Right now, if you enable any image upload functionality without configuring or loading any adapter, there will be no warning on startup. However, an error will be logged if you paste or drop an image and if you try to use the "Insert image" button.

The problem is that the image upload features are enabled by default in all builds. Which means that people may get warnings even though they are not interested in upload at all.

OTOH, silent errors are what developers hate. I can feel how furious I'd be myself when after enabling an UploadImage plugin I'd get no warning or anything when I tried to paste/drop an image. For developers, this is awful. And documentation won't help here – many of the developers will not read it that precisely or at all. Especially that this process is always made of at least two steps – enabling an adapter and configuring it.

Furthermore, there are issues like when should the "Insert image" button log a warning? When added to the toolbar? When used?

Also, a thing to consider is that checking the existence of an adapter in all these features will bloat a code (not in terms of size but complexity). Perhaps it would be wise to add FileLoader#assertLoader() method which would log a standardised warning if needed.

@Reinmar
Copy link
Member Author

Reinmar commented Sep 29, 2017

Actually, the more I think about this the more I think that an error or at least a warning should be logged when trying to paste an image into the editor when a CKE5 build is used.

Why?

Because this is developer's console. If a developer is testing pasting images there's 99% chance that he/she actually wants this image. And the error does not change anything for a normal user because the user doesn't see the console at all.

@Reinmar
Copy link
Member Author

Reinmar commented Sep 29, 2017

Now, funny story – guess who was the first lucky guy who's happy that the error is logged? 😄 👈

I made a typo when configuring the CS integration (which isn't simple, so there's a lot of things to think off) and I realised that the config is not set correctly thanks to that warning. Otherwise, I'd just wonder why nothing's happening after I pasted an image ;)

@Reinmar
Copy link
Member Author

Reinmar commented Apr 5, 2018

This error works fine – we get statistics that people visit its documentation.

@Reinmar Reinmar closed this as completed Apr 5, 2018
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-upload Oct 9, 2019
@mlewand mlewand added resolution:solved status:discussion type:improvement This issue reports a possible enhancement of an existing feature. package:upload labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:upload status:discussion type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

2 participants