Skip to content

Comments

Update slate-drop-or-paste-images to slate to >= 0.43#35

Open
cdunn wants to merge 4 commits intoianstormtaylor:masterfrom
cdunn:update-slate-drop-or-paste-images
Open

Update slate-drop-or-paste-images to slate to >= 0.43#35
cdunn wants to merge 4 commits intoianstormtaylor:masterfrom
cdunn:update-slate-drop-or-paste-images

Conversation

@cdunn
Copy link

@cdunn cdunn commented Nov 19, 2018

  • change -> editor
  • Also add context object to insertImage callback. Useful to access original src for example.

@wmertens
Copy link

@cdunn Hah, I missed this when I made #40. Looks like we took the same approach, I like your additions. What do you think of how I handle the types?

const insertFn = {files: onInsertFiles, html: onInsertHtml, text: onInsertText}
/**
* On drop or paste.
*
* @param {Event} event
* @param {Editor} editor
* @param {Function} next
* @return {State}
*/
function onInsert(event, editor, next) {
const transfer = getEventTransfer(event)
const fn = insertFn[transfer.type]
if (!fn) return next()
const range = getEventRange(event, editor)
return fn(event, editor, next, transfer, range)
}

@wmertens
Copy link

@ianstormtaylor what would it take to merge this? Should the other plugins be upgraded at the same time?


function DropOrPasteImages(options = {}) {
let { insertImage, extensions } = options
let { insertImage, extensions, acceptableTransferTypes } = options

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest more concise name of transferTypes or transfers

plugins = [
DropOrPasteImages({
insertImage: (transform, file) => {
insertImage: (_event, transform, file, _context) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a little fuzzy on the _ naming convention, would recommend removing it but won't fight for it

function onInsertFiles(event, editor, next, transfer, range) {
const { files } = transfer

for (const file of files) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const filteredFiles = !extensions ? files : files.filter(file => {
  const type = file.type
  const [, etx] = type.split('/')
  return ext && matchExt(ext)
})
if (!filteredFiles.length) { return next() }
for (const file of filteredFiles) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest moving the extension checking logic out of the loop. The reason is that this will enable the plugin to return next() if it determines that none of the extensions for the dropped files need to be inserted (IE a no op).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants