U/juliaroldi/image editing refactor #1345
Conversation
juliaroldi
requested review from
JiuqingSong,
BryanValverdeU,
Andres-CT98,
flyingbee2012 and
ianeli1
|
Generally to make the code cleaner, we can deprecate the image selection feature, and always use image selection |
…u/juliaroldi/image-editing-refactor
…u/juliaroldi/image-editing-refactor
juliaroldi
force-pushed
the
u/juliaroldi/image-editing-refactor
branch
from
709e22a to
5de5a1c
Compare
| @@ -34,7 +34,7 @@ export const KnownCreateElementData: Record<KnownCreateElementDataIndex, CreateE | |||
| }, | |||
| [KnownCreateElementDataIndex.ImageEditWrapper]: { | |||
| tag: 'div', | |||
| style: 'width:100%;height:100%;position:relative;overflow:hidden', | |||
| style: 'width:100%;height:100%;position:relative;overflow:hidden;', | |||
There was a problem hiding this comment.
curious: is this change required?
There was a problem hiding this comment.
No, it is not, beautify extension added, because I touched the file. I'll remove.
| @@ -288,7 +310,7 @@ export default class ImageEdit implements EditorPlugin { | |||
| typeof operationOrSelect === 'number' ? operationOrSelect : ImageEditOperation.None; | |||
| const selectImage = typeof operationOrSelect === 'number' ? false : !!operationOrSelect; | |||
|
|
|||
| if (this.image) { | |||
| if (!image && this.image) { | |||
There was a problem hiding this comment.
Why need this check? What if I select another image?
There was a problem hiding this comment.
To avoid a loop. In line 315, the image is being selected again, because we add the wrapper we remove the selection from the image, then we need to select the image again and we do this selection setEditingImage will be called, then to avoid select the image again we do not have the wrapper, unless we use setEditingImage is called with null.
When we trigger mouse we set editing to null, but if we click in another image the imageSelect plugin will select the image and then selection event will be trigger and the new image will be selected.
packages/roosterjs-editor-plugins/lib/plugins/ImageSelection/ImageSelection.ts
Show resolved
Hide resolved
| // Press ESC should cancel current editing operation, resume back to original edit info | ||
| this.editInfo = getEditInfoFromImage(this.image); | ||
| this.setEditingImage( | ||
| e.selectionRangeEx?.image, |
There was a problem hiding this comment.
You already did null check of selectionRangeEx in line 183, no need to add "?" here
| e.source != ChangeSource.InsertEntity || | ||
| (<Entity>e.data).type != IMAGE_EDIT_WRAPPER_ENTITY_TYPE | ||
| (e.source !== ChangeSource.Format && e.source !== ChangeSource.InsertEntity) || | ||
| (<Entity>e.data)?.type != IMAGE_EDIT_WRAPPER_ENTITY_TYPE |
There was a problem hiding this comment.
this need to be combined with the check with ChangeSource.InsertEntity, which means, if e.source is not Insert Entity, or (e.source is InsertEntity, and) entity data type is not image wrapper related.
| @@ -19,7 +19,7 @@ import { | |||
| getEntityFromElement, | |||
| getEntitySelector, | |||
| getObjectKeys, | |||
| matchesSelector, | |||
| //matchesSelector, | |||
There was a problem hiding this comment.
nit: Remove this commented line
| if (onShowResizeHandle) { | ||
| onShowResizeHandle(elementData, x, y) | ||
| onShowResizeHandle(elementData, x, y); |
There was a problem hiding this comment.
elementData can be null, should we add a null check?
There was a problem hiding this comment.
I think this had to be node in another PR to enable strict mode. I didn't made any changes in this files, but the beautify extension add the semicolon
| @@ -280,4 +283,11 @@ export default class CopyPastePlugin implements PluginWithState<CopyPastePluginS | |||
| table.style.removeProperty('height'); | |||
| } | |||
| } | |||
|
|
|||
| private deleteImage(editor: IEditor, imageId: string) { | |||
| const image = editor.getDocument().querySelector('#' + imageId); | |||
There was a problem hiding this comment.
use editor.queryElements() instead to avoid query image outside editor
| break; | ||
|
|
||
| case PluginEventType.MouseDown: | ||
| this.setEditingImage(null); |
There was a problem hiding this comment.
This works. But I think that is what I concerned about current implementation. We can go with this for now, and revisit it later.
There was a problem hiding this comment.
I think we can change this implementation, but we would have to change the select function to trigger selection change event, when no range is selected.
Adapt Image Edit plugin to use Selection Change Event.