This repository has been archived by the owner on Jun 26, 2020. It is now read-only.
T/42: Various issues when dropping or inserting files. SRP in the command #47
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…e features use it.
…uperfluous tests and corrected test for inserting image at disallowed position.
Reinmar
changed the title
pjasiun
reviewed
Jul 27, 2017
src/utils.js
Outdated
| @@ -19,3 +21,47 @@ export function isImageType( file ) { | |||
| return types.test( file.type ); | |||
| } | |||
|
|
|||
| /** | |||
| * Returns a model position which is optimal (in terms of UX) for inserting and image. | |||
There was a problem hiding this comment.
"an image" ofc. I'll fix it.
pjasiun
reviewed
Jul 27, 2017
src/utils.js
Outdated
| /** | ||
| * Returns a model position which is optimal (in terms of UX) for inserting and image. | ||
| * | ||
| * For instance, if a selection is in a middle of a paragraph, position before this paragraph |
There was a problem hiding this comment.
"For instance", and then you described all cases ;)
There was a problem hiding this comment.
No, I didn't. Element selection, edge cases like no blocks, etc.
pjasiun
reviewed
Jul 27, 2017
| const selectedElement = selection.getSelectedElement(); | ||
|
|
||
| if ( selectedElement ) { | ||
| return ModelPosition.createAfter( selectedElement ); |
There was a problem hiding this comment.
The code reads so easily that I don't know what else you can write. If there's some element selected, return position after this selected element. It's nearly 1:1 to natural language :D
|
The code looks and works great. After you update docs and comments you can merge this PR. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Suggested merge commit message (convention)
Fix: Fixed two issues related to dropping images. First, when dropping a file into an empty paragraph, that paragraph should be replaced with that image. Second, drop position should be read correctly when the editor is focused upon drop. Closes ckeditor/ckeditor5#2814. Closes ckeditor/ckeditor5#2802.
BREAKING CHANGE:
UploadImageCommanddoesn't optimize the drop position itself anymore. Instead, a separatefindOptimalInsertionPosition()function was introduced.BREAKING CHANGE:
UploadImageCommanddoesn't verify the type of file anymore. This needs to be done by the caller.Additional information
I started with minimal changes, but it soon turned out that they lead to a lot more changes. I added some missing tests and it's possible that I actually fixed a few more bugs, but I didn't check if those things didn't work in the past.