T/1 Upload initial implementation. #11
Conversation
src/filerepository.js
Outdated
| * @inheritDoc | ||
| */ | ||
| static get pluginName() { | ||
| return 'fileRepository'; |
There was a problem hiding this comment.
@Reinmar: according to the recent convention should it be "upload/fileRepository"? I must say I don't like these plugins namespaces. They make names much longer does not let overwrite plugins nicely in the future and might be a problem if we will try to move plugins between packages (note that from the end-developer perspective it will be the only thing which will change).
There was a problem hiding this comment.
according to the recent convention should it be "upload/fileRepository"? I
That's not the recent convention. That's the convention we had from the beginning. And it should be the module name – upload/filerepository.
They make names much longer
Really? :D I counted 7 characters here.
does not let overwrite plugins nicely in the future and might be a problem if we will try to move plugins between packages (note that from the end-developer perspective it will be the only thing which will change).
If we'll move plugins between repositories we'll have a lot more issues.
We scope packages for the same reason why we scope packages – to avoid accidental conflicts (although, in case of plugins we're scoping by the pkg name, not the org name, because, if we added org name, then, indeed, the name would be awfully long). Your plugin may work with x/y but it may not work with x2/y if one will load it to his/her editor. So if you retrieved the plugin by y you'd blow up the whole thing.
If, for some reason, someone will ever decide to write a copy of plugin x/y which is supposed to replace it, then he/she can name it like this. But this should be intentional, not accidental.
PS. Strategy for naming contributing plugins is yet to be defined.
There was a problem hiding this comment.
But at the same time, you have 'bold' instead of 'basicstyles/bold' in the toolbar. What's the difference?
There was a problem hiding this comment.
That's a good question :P We'll have to propose a convention there too, so contrib stuff doesn't conflict. But you're right that it should be the same convention.
We have two choices – to either say that only our stuff doesn't have to be prefixed. Or to always prefix our stuff with ckeditor or cksource. This will look ridiculous though if you have ckeditor/bold or something. I'll open a ticket to review this generally. For now, just please follow the current convention.
| adapterMock.mockSuccess( { original: './sample.jpg' } ); | ||
| } | ||
| } | ||
| } ); |
There was a problem hiding this comment.
I wanted to say that I miss "error" button there, but since upload does not handle upload yet, it does not make much sense. It should be added later.
src/imageuploadengine.js
Outdated
| @@ -0,0 +1,144 @@ | |||
| /** | |||
| * Copyright (c) 2016, CKSource - Frederico Knabben. All rights reserved. | |||
There was a problem hiding this comment.
Use:
* @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md.
src/filerepository.js
Outdated
| * will be passed to that function. | ||
| * | ||
| * fileRepository.createAdapter = function( loader ) { | ||
| * return new ServerAdapter(); |
There was a problem hiding this comment.
It's not very useful for someone who doesn't know how ServerAdapter should look like and someone who knows how ServerAdapter should look like should also know about this. I think it will be more useful for the developer to keep both together.
There was a problem hiding this comment.
I mean you can move this to the doc about the Adapter interface and put the link here only.
src/filerepository.js
Outdated
| this.set( 'uploaded', 0 ); | ||
|
|
||
| /** | ||
| * Number of total bytes to upload. It contains `null` if value is not available yet. |
There was a problem hiding this comment.
The note that uploadTotal might be different that the file size (actually bigger because of headers and additional data), would be useful.
There was a problem hiding this comment.
Also Note: Because total can be nulluseuploadedPercent to show progress.
src/filerepository.js
Outdated
|
|
||
| /** | ||
| * Aborts the upload process. | ||
| * After aborting it should reject promise returned from {@link #upload upload()} method with "aborted" string. |
There was a problem hiding this comment.
There is no return in the sample. And I'm not sure if it is needed. If we assume that abort is always executed by the loader.abort() method, the status will change to aborted and the adapter developer should not need to do anything more.
src/imageuploadengine.js
Outdated
| * | ||
| * @member {String} #placeholder | ||
| */ | ||
| this.placeholder = 'data:image/svg+xml;utf8,' + uploadingPlaceholder; |
There was a problem hiding this comment.
It can be protected at this stage. We can make it public when we will find it needed.
| * @param {module:engine/model/batch~Batch} batch | ||
| * @param {module:engine/model/element~Element} imageElement | ||
| */ | ||
| load( loader, batch, imageElement ) { |
src/filerepository.js
Outdated
|
|
||
| /** | ||
| * Creates loader for specified file. | ||
| * Shows console warning and returns `null` if {@link #createAdapter} method is not defined.. |
Suggested merge commit message (convention)
Feature: Initial implementation. Closes ckeditor/ckeditor5#2784.
Additional information
Needs ckeditor/ckeditor5-clipboard#17.