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

Fix file paste: cache file info #7156

Merged
merged 3 commits into from Jun 8, 2018

Conversation

Projects
None yet
2 participants
@iseulde
Member

iseulde commented Jun 5, 2018

Description

See #6960. This change is causing errors for me when copy pasting images in Chrome.

How has this been tested?

  • Copy paste an image into Gutenberg.
  • Drap and drop an image into Gutenberg.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
return url;
}
export function getBlobByURL( url ) {
if ( cache[ url ] ) {
return Promise.resolve( cache[ url ] );
const { blob, name, lastModified, type } = cache[ url ];
const file = new File( [ blob ], name, { lastModified, type } );

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 5, 2018

Member

Would caching new File( [ blob ], name, { lastModified, type } ); directly in cache[ url ] be an option?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 5, 2018

Member

e.g. in createBlobURL do cache[ url ] = new File( [ blob ], name, { lastModified, type } );

This comment has been minimized.

@iseulde

iseulde Jun 6, 2018

Member

Sure, sounds good

@jorgefilipecosta

This seems to solve the problem happening on copy paste. When using drag&drop the images continue to keep their file names. The code becomes better with this changes an awesome advantage of now using createBlobURL instead of createObjectURL.
Thank you a lot for this fix!

@iseulde

This comment has been minimized.

Member

iseulde commented Jun 6, 2018

I can't recall why the fetch was added in #2792... Perhaps a fall back if there's no cache left or it's somehow copy pasted, but I don't see a specific use case after searching a while. The problem with this line is that we can't set a mime type and it won't go through anyway, so I'm going to remove it.

@iseulde iseulde requested review from jorgefilipecosta and WordPress/gutenberg-core Jun 8, 2018

@iseulde iseulde added this to the 3.1 milestone Jun 8, 2018

@iseulde

This comment has been minimized.

Member

iseulde commented Jun 8, 2018

@jorgefilipecosta Does everything still work for you with the latest change?

}
return fetch( url ).then( ( response ) => response.blob() );
return cache[ url ];

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 8, 2018

Member

Before we verified if the file exists in the cache, is it safe to assume that for all blob url's used here we used createBlobURL before? In that case, I think we should specify in the comments that this function only works for blubUrls created by createBlobURL.

This comment has been minimized.

@iseulde

iseulde Jun 8, 2018

Member

Sure, that hasn't changed. I'll add a comment.

@jorgefilipecosta

I tested drag&drop with single & multiple images the names were kept with success.
Copy & paste for image files works great the names are not kept but I think it is not possible to access the original file name during a copy paste.

I think we should add comment in getBlobByURL to specify it only works for blob urls created by createBlobUR. But besides that everything looks good to me.
Nice work, thank you for this PR 👍

@iseulde iseulde merged commit 54d405a into master Jun 8, 2018

2 checks passed

codecov/project 46.68% (-0.01%) compared to a9fab4d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@iseulde iseulde deleted the fix/file-paste branch Jun 8, 2018

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