Implement File block #6805

mirka · 2018-05-17T19:06:20Z

Closes #5462

Description

Implements a File block with the following features:

How has this been tested?

Try the features above.

Screenshots

Things I don't plan on including in this PR (but should be done eventually)

~~Integrate error messaging using Fix max upload size error message #4203~~ done, thanks to Try unifying all the media blocks placeholder under a unique component #6820
~~Add allowed mime type checking to Fix max upload size error message #4203~~ submitted separately as Check allowed mime types before uploading media #6968
~~Refactor the misplaced CSS in core-blocks/image/editor.scss~~ done, thanks to Try unifying all the media blocks placeholder under a unique component #6820
Add drag and drop support to Audio/Video blocks so they can take precedence over this generic File block
~~Copy URL button may not work correctly in Safari~~ submitted separately as Make ClipboardButton inside a block work correctly in Safari #7106

jasmussen · 2018-05-18T07:07:44Z

Tried to check this out and run it. VERY cool work! Very very nice, definitely getting there.

A few niggles:

Can we make it so you can drag and drop a PDF file directly into the canvas, in addition to on the file block placeholder?

It would be nice if the Download button text were editable. Perhaps even that you could toggle off the download button entirely in the sidebar. If you remove the actual link from the button inside the editor, so you can't download from inside the editor but only on the published front-end, then you could make the button clickable to edit, just like the normal Button block.

Given that you can edit the text of the link, right now the "Edit" button taking you back to the placeholder feels a little bit jarring. Could you make it so instead of having the layout alignment buttons and an edit button, it has instead text-block-like editing tools?

Instead of:

You'd have:

This would essentially be like a text field with a button. You could write your own text, link, unlink, bold, italic, etc. You'd never be able to go back to the placeholder block from the final state, but that's fine too, and the button would always hold the link. What do you think?

I notice by the way that if you edit the link, you're typing in reverse :) — but I'll bet that's what you refer to in the remaining bullet.

Background Color and Text Color options in sidebar

We probably don't need these, but nice idea otherwise!

Very cool work, thanks for working on this, and keep up he good work!

mirka · 2018-05-18T15:53:46Z

Thanks for the review, @jasmussen!

Can we make it so you can drag and drop a PDF file directly into the canvas, in addition to on the file block placeholder?

Do you mean like this?

If so, it is already supposed to be working. You might be seeing a bug. Could you give me some more details so I can reproduce it?

~~(BTW are you rebasing onto a branch that is not master? It looks like you have icons in the toolbar that I don't)~~ I get it now 🙂

It would be nice if the Download button text were editable. Perhaps even that you could toggle off the download button entirely in the sidebar.

Sure! We could do that.

This would essentially be like a text field with a button. You could write your own text, link, unlink, bold, italic, etc. You'd never be able to go back to the placeholder block from the final state, but that's fine too, and the button would always hold the link. What do you think?

Hmm, I thought about this too. At one point I even considered a hybrid approach:

(I scrapped this idea, because having a link icon there would make the pencil icon a lot less discoverable)

My main concern is that we’d confuse the user’s mental model of where the “source of truth” for the file link is.

Since choosing a file from the placeholder sets both links, you can reasonably assume that both links will change if you choose another file from the same interface:

But when you introduce a second way to change the link, it’s harder to tell what’s going to happen. If only the text link were to change, it would become hard to tell that the text link and button link are out of sync. On the other hand, if both links were to change, it would break the expectation of the standard rich text UI:

Is this a tradeoff we are willing to take? Let me know what you think 🙂

ZebulanStanphill · 2018-05-18T17:58:51Z

(BTW are you rebasing onto a branch that is not master? It looks like you have icons in the toolbar that I don't)

Those are the wide and full-width alignment options, which do not show unless your theme declares support for them, since those kinds of alignments extend out of the standard content width and may not look right depending on the theme. Since Gutenberg is so new, there are very few themes that have declared support for those alignment types, so most of the time you will only see those options if you use a custom child theme which has declared support for them.

mirka · 2018-05-18T18:01:56Z

@SuperGeniusZeb Oh I see, that makes sense now. Thank you!

…6805

Placeholder text state was not properly updating when uploading a file from the media placeholder.

jasmussen · 2018-05-28T07:39:15Z

Very nice work! Here's a GIF of it:

Experience wise, this seems to behave as we've discussed, and given the paradigms Gutenberg works with, this seems like it solves the task at hand:

allows file uploads
allows inserting files easily
allows copying the link so you can use it to create a more regular in-text link

For the scope of this task, 👍 👍, I think this is cool! Nice work.

Did you include/merge the work from master that unifies the placeholder block?

At some point in the future, as a separate project to this, I imagine that we'll want to take a look at the media library in WordPress in general. In doing so, there will likely also be an opportunity to look at how the placeholder block can work. For example it would be nice if we could show part of the media library itself inside the placeholder block, and allow you to simply drag into it there, and then insert as file block or insert as plaintext link. But that's something to consider for the future.

mirka · 2018-05-28T20:04:34Z

@jasmussen Thank you for all your guidance, and the cool gif! There are so many possibilities with the Gutenberg framework, and I'm excited to see how the UX will evolve. Hope I'll have a chance to contribute more.

Did you include/merge the work from master that unifies the placeholder block?

Yes. The code is a lot cleaner thanks to that. And there's another modification in the works (#6957), so if that gets merged in first I'll get to delete a few more lines 🙂

nb · 2018-05-29T11:29:00Z

components/clipboard-button/index.js

+		// the document.activeElement at the moment when the copy event fires.
+		// This causes documentHasSelection() in the copy-handler component to
+		// mistakenly override the ClipboardButton, and copy a serialized string
+		// of the current block instead.


Thanks for the detailed comment, explaining why do we have the workaround.

One problem in Safari, though, is that the while block (<!-- wp:file {"href":…) ends up in the clipboard.

Ok, it looks like this workaround hadn't fixed the Safari bug completely. I did some testing and narrowed down the repro condition a bit. My workaround seems to be effective for pre-existing posts (i.e. posts that were already saved when the page loaded), but not for new posts. Not sure what's going on, but that's a start... I'll have to look into it further.

Addressed in #7106

nb · 2018-05-29T11:42:03Z

core-blocks/file/edit.js

+			getBlobByURL( href )
+				.then( ( file ) => {
+					editorMediaUpload( {
+						allowedType: '*',


Just to make sure I understand correctly, once #6968 gets in we will feed the list if types here?

No, that allowedType is being used for limiting the files to a suggested file type, similar to the accept attribute of an <input type="file">. The naming is very confusing right now. I want to rename that to accept (and maybe enhance it to take comma-separated and file extension arguments like the actual <input accept>) in a separate PR once this and #6968 are merged in.

From #5462:

WordPress also supports a number of filetypes that aren't media, such as pdf, doc, and others. See full list here: https://codex.wordpress.org/Uploading_Files

I don't think that editorMediaUpload in the current shape is what we really want to use in here, because non-media files can be uploaded, too. It seems like we need to refactor the existing method or add a more generic one which editorMediaUpload uses behind the scenes.

Hmm, I was under the impression that the "media" in editorMediaUpload and mediaUpload is being used in the broader sense of the word, and not to mean "images, audio, and video". This is in line with how WordPress uses the term — every kind of uploadable file, including pdf and other documents, are uploaded to the "Media Library", and are retrieved via the media endpoint.

Even /edit-post/hooks/blocks/media-upload, which includes specific logic for when gallery == true, seems generic enough for any kind of file. Is there a reason we want to keep editorMediaUpload specifically for image/audio/video files?

I posted a separate issue for the allowedType problem: #7155

nb · 2018-05-29T11:44:08Z

core-blocks/file/edit.js

+		}
+	}
+
+	isBlobURL( url = '' ) {


Would it make sense to move this one to utils, like the other blob functions? At which point would you split out utils/blob?

Yes, I think so. But in a separate PR, because I think I saw a couple of other places in the codebase where we’re doing blob url detection, and I’d want to change them all at once.

nb · 2018-05-29T11:51:10Z

core-blocks/file/index.js

+					<a
+						href={ textLinkHref }
+						target={ openInNewWindow }
+						rel={ openInNewWindow ? 'noreferrer noopener' : false }


Why the noreferrer?

I think it’s because noopener doesn’t work in IE/Edge. (see also: #6186)

nb · 2018-05-29T12:04:40Z

core-blocks/file/edit.js

+
+		// edit component has its own attributes in the state so it can be edited
+		// without setting the actual values outside of the edit UI
+		this.state = {


I am not sure I follow the reason we keep the attributes in the state, too. What do we gain?

You're right, I've cleaned them out.

nb · 2018-05-29T12:11:48Z

core-blocks/file/edit.js

+
+		if ( id !== undefined ) {
+			// Get Attachment Page URL
+			wp.apiRequest( {


Any reason we’re not using withAPIData here?

I didn't realize we had that! Cool. I put that in place, however it turns out that the snapshot test helper for our components (blockEditRender()) isn't equipped to inject context in order for it to work. I've made a patch for it and I'll propose it in a separate PR, but not sure if my implementation is good. Anyway, I'm going to remove the snapshot test for this temporarily.

nb · 2018-05-29T12:14:48Z

core-blocks/file/edit.js

+			} )
+				.then(
+					( result ) => {
+						this.setState( { attachmentPage: result.link } );


Would the setState work correctly if the component hasn't been rendered yet?

Good point. Should've been in componentDidMount().

nb · 2018-05-29T12:21:34Z

core-blocks/file/edit.js

+
+	componentDidUpdate( prevProps ) {
+		// Reset copy confirmation state when block is deselected
+		if ( prevProps.isSelected && ! this.props.isSelected ) {


This worked great for me as a user.

nb · 2018-05-29T12:36:11Z

core-blocks/file/editable-link.js

+
+	componentDidUpdate( prevProps ) {
+		if ( prevProps.text !== this.props.text ) {
+			this.setState( { showPlaceholder: ! this.props.text } );


What are the trade-offs of using state here vs. just using the text prop for rendering logic?

Since I'm resorting to use a content-editable here, this is not a "controlled" component and therefore the text prop isn't updated on each keystroke.

Requires a modification in the `blockEditRender()` test helper to inject context, or else the withAPIData HOC won’t work.

…file-block # Conflicts: # utils/mediaupload.js

nb · 2018-06-04T10:00:50Z

core-blocks/file/edit.js

-			this.setState( {
-				attachmentPage: media.link,
-			} );
+			}, this.props.media.get /* refetch attachment page url */ );


Does setAttributes accept a second argument? When is this.props.media.get called?

Totally assumed setAttributes took the same arguments as setState! I meant it as a callback. Thank you.

gziolo · 2018-06-05T08:21:22Z

core-blocks/file/edit.js

+
+export default compose( [
+	withAPIData( ( props ) => ( {
+		media: `/wp/v2/media/${ props.attributes.id }`,


You should be rather using getMedia selector instead of withAPIData which is going to be deprecated as soon as we find a way to port all existing code that handles REST API requests. This will also remove the need to introducing changes proposed in #7108.

export default compose( [ withSelect( ( select, props ) => { const { getMedia } = select( 'core' ); return { image: getMedia( props.attributes.id ), // not sure if getMedia handles undefined values }; } ), withNotices, ] )( FileEdit );

Thank you, I wasn't aware of this. I'll look into it.

gziolo · 2018-06-05T08:30:44Z

utils/mediaupload.js

@@ -33,7 +33,9 @@ export function mediaUpload( {
 		filesSet[ idx ] = value;
 		onFileChange( compact( filesSet ) );
 	};
-	const isAllowedType = ( fileType ) => startsWith( fileType, `${ allowedType }/` );
+	const isAllowedType = ( fileType ) => {
+		return ( allowedType === '*' ) || startsWith( fileType, `${ allowedType }/` );


Should it rather list all allowed file types by allowing the array format?

Yes, I think it should accept arrays, as well as have a more predictable syntax. More on that here: #7155

I opted for a wildcard here as a stop-gap measure, or else we'd have something like ['audio', 'application', 'video', 'text', 'image', 'font'] which I think is more error-prone for consumers.

mirka · 2018-06-27T21:35:46Z

By the way, here are some of the RichText issues that caused me to resort to a contenteditable for the text link part: #7575, #5657, #7591.

Now that I've sorted through the repro conditions and workarounds for them, I feel like #7591 is the only "blocker" for using a RichText here. If that is addressed, I think we could safely replace the contenteditable.

noisysocks · 2018-06-29T02:17:59Z

Thanks for your excellent work on this @mirka. @talldan and I are going to pick up on this in #7622.

mirka added 25 commits May 16, 2018 04:46

Add basic file block

8459693

Reset id attribute when new URL is specified

263e817

Remove extra margin on file block placeholder

9dfb13f

Upload files dropped into editor

602e2be

Add transient animation while uploading file

d528d4c

Don’t call string method on undefined url

00b7b7a

Download file with filename

2dec5a4

Add option to link to attachment page

cab2dde

Only disassociate id when URL was changed

5285db7

Split out file block inspector controls

d2b7a67

Add option to open in new window

f5baad2

Make filename editable

b924e00

Add hover style to button

b261b8e

Only render when href is not empty

0a122e2

Add noreferrer noopener to target _blank links

b82cb37

Make text links copy-pasteable

423f4b4

Allow empty filenames

d4bb0f3

Add DropZone to file block

826be70

Make file block transformable to/from audio/video

6e8ef78

Fix cursor disappearing while editing text link

0f78b07

Filter out premature onChange events in CJK

1407609

Tweak code readability

938246f

Fix line break filtering

e4ce6b4

Remove unnecessary RichText component in save()

5cca7d2

Add tests

b415e7c

Merge branch 'master' of https://github.com/WordPress/gutenberg into …

479ccf4

…6805

Update tests

ba01c11

mirka changed the title ~~[WIP] Implement File block~~ Implement File block May 26, 2018

mirka mentioned this pull request May 27, 2018

Check allowed mime types before uploading media #6968

Merged

4 tasks

Fix placeholder text toggling bug

c91fe9c

Placeholder text state was not properly updating when uploading a file from the media placeholder.

nb reviewed May 29, 2018

View reviewed changes

mirka added 4 commits May 31, 2018 03:59

Remove unnecessary state

0be3946

Use withAPIData HOC

677b35c

Remove snapshot testing for now

f19973b

Requires a modification in the `blockEditRender()` test helper to inject context, or else the withAPIData HOC won’t work.

Merge branch 'master' of https://github.com/WordPress/gutenberg into …

f07f213

…file-block # Conflicts: # utils/mediaupload.js

mirka mentioned this pull request Jun 3, 2018

Make ClipboardButton inside a block work correctly in Safari #7106

Merged

nb reviewed Jun 4, 2018

View reviewed changes

gziolo reviewed Jun 5, 2018

View reviewed changes

gziolo mentioned this pull request Jun 5, 2018

Allow context injection in core-block tests #7108

Closed

gziolo reviewed Jun 5, 2018

View reviewed changes

Use withSelect instead of withAPIData

80c802e

mtias added the New Block Suggestion for a new block label Jun 19, 2018

noisysocks mentioned this pull request Jun 29, 2018

Implement File block #7622

Merged

noisysocks closed this Jun 29, 2018

Implement File block #6805

Implement File block #6805

Conversation

mirka commented May 17, 2018 • edited Loading

Description

How has this been tested?

Screenshots

Things I don't plan on including in this PR (but should be done eventually)

jasmussen commented May 18, 2018

mirka commented May 18, 2018 • edited Loading

ZebulanStanphill commented May 18, 2018 • edited Loading

mirka commented May 18, 2018

jasmussen commented May 28, 2018

mirka commented May 28, 2018

Choose a reason for hiding this comment

nb May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirka commented Jun 27, 2018

noisysocks commented Jun 29, 2018

mirka commented May 17, 2018 •

edited

Loading

mirka commented May 18, 2018 •

edited

Loading

ZebulanStanphill commented May 18, 2018 •

edited

Loading

nb May 29, 2018 •

edited

Loading

gziolo Jun 5, 2018 •

edited

Loading

gziolo Jun 5, 2018 •

edited

Loading