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

Implement File block #7622

Merged
merged 20 commits into from Jul 3, 2018

Conversation

@noisysocks
Member

noisysocks commented Jun 29, 2018

Based off of @mirka's work in #6805.

Closes #5462.

Implements a File block with the following features:

  • Editable text link
  • Text link can be copy and pasted to create a normal link
  • HTML5 download link
  • Copy URL button to copy the media file URL to clipboard
  • Accepts both Media Library files and external URLs only Media Library files
  • Text link can be changed to point to the Attachment Page
  • Text link can be changed to open in a new window
  • Accepts and uploads a file drag-and-dropped into the editor (or placeholder)
  • Transforms to/from Audio and Video blocks
  • Download button text is editable
  • Download button is optional
Implement File block
Implements a File block with the following features:

- Editable text link
- Text link can be copy and pasted to create a normal link
- HTML5 download link
- Copy URL button to copy the media file URL to clipboard
- Accepts both Media Library files and external URLs only Media Library files
- Text link can be changed to point to the Attachment Page
- Text link can be changed to open in a new window
- Accepts and uploads a file drag-and-dropped into the editor (or placeholder)
- Transforms to/from Audio and Video blocks
- Download button text is editable
- Download button is optional

Co-authored-by: mirka <mirka@jaguchi.com>

@noisysocks noisysocks referenced this pull request Jun 29, 2018

Closed

Implement File block #6805

11 of 11 tasks complete
export const settings = {
title: __( 'File' ),
description: __( 'Link to a file.' ),

This comment has been minimized.

@noisysocks

noisysocks Jun 29, 2018

Member

@karmatosed — any suggestions for what the File block's description should be?

This comment has been minimized.

@mcsf

mcsf Jun 29, 2018

Contributor

Something closer to Serve / put up a file for download?. Add a link to a file that visitors can download?

transform: ( attributes ) => {
return createBlock( 'core/file', {
href: attributes.src,
fileName: attributes.caption && attributes.caption.join(),

This comment has been minimized.

@noisysocks

noisysocks Jun 29, 2018

Member

Calling join() like this won't work for more complex cases where we have e.g. bold and italic text in the caption.

This comment has been minimized.

@talldan

talldan Jun 29, 2018

Contributor

Created issue #7626 for this.

@talldan

This comment has been minimized.

Contributor

talldan commented Jun 29, 2018

It seems possible to transform file blocks that are not audio or video to those block types (e.g. a pdf).

edit - created issue #7625

@noisysocks

This comment has been minimized.

Member

noisysocks commented Jun 29, 2018

This is in pretty good shape now. @talldan and reviewed and tested the code together, and fixed up some minor issues. Aiming to merge this early next week.

@Soean

This comment has been minimized.

Member

Soean commented Jun 29, 2018

Nice work! What do you think about selecting/changing the color of the download button?

@mcsf

This looks great, even though I left some generally secondary comments.

The 'file' transform I'd have a better look at, either fixing or removing it.

export const settings = {
title: __( 'File' ),
description: __( 'Link to a file.' ),

This comment has been minimized.

@mcsf

mcsf Jun 29, 2018

Contributor

Something closer to Serve / put up a file for download?. Add a link to a file that visitors can download?

* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { createBlobURL } from '@wordpress/utils';

This comment has been minimized.

@mcsf

mcsf Jun 29, 2018

Contributor

Deprecated in favor of wp.blob.

},
},
],
to: [

This comment has been minimized.

@mcsf

mcsf Jun 29, 2018

Contributor

It's pretty strange to unconditionally offer transforms to audio and video regardless of file type, but I also understand that there's no matching mechanism in transforms (unlike isMatch when converting classic content to blocks). We could tackle this later, but just noting that the UX can be a bit weird (offering a transform that will break if the file is, say, a ZIP file).

This comment has been minimized.

@talldan

talldan Jul 2, 2018

Contributor

Yep, agree with that. I've created an issue for this - #7625.

The question is whether we're happy to leave it as it is and merge this, or whether we want to remove the transforms temporarily.

Another alternative could be to show a warning on audio and video blocks if the file type is incorrect.

This comment has been minimized.

@noisysocks

noisysocks Jul 2, 2018

Member

I've no strong opinion either way. Leaning slightly to leaving it as is for now because if we fix the bug in a future PR it aligns better with our goal of having the next release be 'feature complete'.

@@ -14,7 +14,7 @@ import { mediaUpload } from '@wordpress/utils';
* Wrapper around mediaUpload() that injects the current post ID.
*
* @param {Object} $0 Parameters object passed to the function.
* @param {string} $0.allowedType The type of media that can be uploaded.
* @param {string} $0.allowedType The type of media that can be uploaded, or '*' to allow all.

This comment has been minimized.

@mcsf

mcsf Jun 29, 2018

Contributor

Is it better to require an explicit '*' or to make allowedType optional and assume no restriction? (I'm not sure)

This comment has been minimized.

@talldan

talldan Jul 2, 2018

Contributor

I had a think about it, and I think I prefer the * over making it optional. It at least signals a clear intention and is an accepted wildcard.

Making it optional and leaving it undefined could indicate either any or no allowed types.

Another option is to use a constant for the accepted values - e.g allowedType: fileTypes.image or allowedType: fileTypes.any.

href: blobURL,
fileName: file.name,
textLinkHref: blobURL,
} );

This comment has been minimized.

@mcsf

mcsf Jun 29, 2018

Contributor

Here's what I got when dragging a small ZIP file into the editor:

screen shot 2018-06-29 at 14 56 23

This comment has been minimized.

@talldan

talldan Jul 2, 2018

Contributor

Fixed in 5202a06

talldan added some commits Jul 2, 2018

Fix error with file block when dragging/dropping file
When dragging and dropping a file to create a new block an error
was being thrown. This is caused by attempting to use `getBlobByUrl`
as though it returns a promise. This function seems to be synchronous
so the fix is to use it that way.
@noisysocks

This comment has been minimized.

Member

noisysocks commented Jul 3, 2018

Thanks for addressing those comments @talldan. This looks good to me! 👍:shipit:

@pento

pento approved these changes Jul 3, 2018

Looks good to me, let's ship it!

Fun side note, I was initially running into max file size issues, which I thought had been fixed. We really should get #4203 merged, too.

@noisysocks noisysocks merged commit 9356304 into master Jul 3, 2018

2 checks passed

codecov/project 46.66% (-0.33%) compared to 345e1ea
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@noisysocks noisysocks deleted the add/file-block branch Jul 3, 2018

@noisysocks

This comment has been minimized.

Member

noisysocks commented Jul 3, 2018

Thanks @pento! And props again to @mirka for the great work here 🎉🌈💃

@talldan

This comment has been minimized.

Contributor

talldan commented Jul 3, 2018

and also thanks @mcsf for the helpful review comments 👍

@gziolo gziolo referenced this pull request Jul 3, 2018

Closed

Add an "Insert File" Block #7683

transforms: {
from: [
{
type: 'files',

This comment has been minimized.

@aduth

aduth Jul 18, 2018

Member

Seems to me this should have been given a priority of low value, since as implemented it has identical priority as any other file transform, when in fact this should be the fallback. For example, dropping an image should take advantage of the Image block's files transform before this one. I don't know if this is currently working, and if it is it only incidental and prone to future breakage.

See: https://wordpress.org/gutenberg/handbook/block-api/#transforms-optional

To control the priority with which a transform is applied, define a priority numeric property on your transform object, where a lower value will take precedence over higher values. This behaves much like a WordPress hook. Like hooks, the default priority is 10 when not otherwise set.

This comment has been minimized.

@aduth

aduth Jul 18, 2018

Member

Issue at #8022

onCopy={ this.copyLinkToClipboard }
onCut={ this.copyLinkToClipboard }
onPaste={ this.forcePlainTextPaste }
contentEditable

This comment has been minimized.

@aduth

aduth Jul 18, 2018

Member

Warning:

Warning: A component is contentEditable and contains children managed by React. It is now your responsibility to guarantee that none of those nodes are unexpectedly modified or duplicated. This is probably not intentional.

Should this be using the RichText component instead? There are many complexities of contentEditable that are meant to be abstracted into this component for general usage like this.

This comment has been minimized.

@aduth

aduth Jul 18, 2018

Member

Issue at #8023

@aduth

This comment has been minimized.

Member

aduth commented Jul 18, 2018

This block appears to completely fail to handle file types not supported for upload. See #8025.

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