Skip to content
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 #6805

Closed
wants to merge 50 commits into from
Closed
Show file tree
Hide file tree
Changes from 47 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
8459693
Add basic file block
mirka May 12, 2018
263e817
Reset id attribute when new URL is specified
mirka May 12, 2018
9dfb13f
Remove extra margin on file block placeholder
mirka May 12, 2018
602e2be
Upload files dropped into editor
mirka May 12, 2018
d528d4c
Add transient animation while uploading file
mirka May 12, 2018
00b7b7a
Don’t call string method on undefined url
mirka May 12, 2018
2dec5a4
Download file with filename
mirka May 12, 2018
cab2dde
Add option to link to attachment page
mirka May 13, 2018
5285db7
Only disassociate id when URL was changed
mirka May 13, 2018
d2b7a67
Split out file block inspector controls
mirka May 13, 2018
f5baad2
Add option to open in new window
mirka May 13, 2018
b924e00
Make filename editable
mirka May 13, 2018
b261b8e
Add hover style to button
mirka May 13, 2018
0a122e2
Only render when href is not empty
mirka May 13, 2018
b82cb37
Add noreferrer noopener to target _blank links
mirka May 13, 2018
423f4b4
Make text links copy-pasteable
mirka May 13, 2018
d4bb0f3
Allow empty filenames
mirka May 13, 2018
826be70
Add DropZone to file block
mirka May 14, 2018
6e8ef78
Make file block transformable to/from audio/video
mirka May 14, 2018
0f78b07
Fix cursor disappearing while editing text link
mirka May 14, 2018
1407609
Filter out premature onChange events in CJK
mirka May 14, 2018
938246f
Tweak code readability
mirka May 15, 2018
e4ce6b4
Fix line break filtering
mirka May 15, 2018
5cca7d2
Remove unnecessary RichText component in save()
mirka May 15, 2018
b415e7c
Add tests
mirka May 15, 2018
479ccf4
Merge branch 'master' of https://github.com/WordPress/gutenberg into …
mirka May 19, 2018
6083fa2
Make download button optional
mirka May 19, 2018
1a8020c
Make button text editable
mirka May 19, 2018
f9bde62
Merge branch 'master' of https://github.com/WordPress/gutenberg into …
mirka May 23, 2018
bda121d
Merge branch 'master' of https://github.com/WordPress/gutenberg into …
mirka May 24, 2018
b628fb8
Replace with unified media placeholder
mirka May 24, 2018
99d28b2
Simplify text link editing
mirka May 24, 2018
4aa49aa
Fix drag-and-drop uploading
mirka May 24, 2018
3df9827
Clean up CSS
mirka May 25, 2018
6aa6f60
Add Copy URL button
mirka May 25, 2018
d2874ca
Fix placeholder not appearing when backspaced
mirka May 25, 2018
aba955f
Make text link copy to clipboard as rich text
mirka May 26, 2018
8325413
Use custom text link component instead of RichText
mirka May 26, 2018
17d5a00
Disable multiline editing for download button
mirka May 26, 2018
41b6309
Clarify prop passing to inspector
mirka May 26, 2018
4bfe800
Delegate confirm copy timeouts to ClipboardButton
mirka May 26, 2018
d2e95c7
Fix overridden Copy URL in Safari
mirka May 26, 2018
768a46d
Merge branch 'master' of https://github.com/WordPress/gutenberg into …
mirka May 26, 2018
ba01c11
Update tests
mirka May 26, 2018
c91fe9c
Fix placeholder text toggling bug
mirka May 27, 2018
0be3946
Remove unnecessary state
mirka May 30, 2018
677b35c
Use withAPIData HOC
mirka May 30, 2018
f19973b
Remove snapshot testing for now
mirka May 31, 2018
f07f213
Merge branch 'master' of https://github.com/WordPress/gutenberg into …
mirka Jun 3, 2018
80c802e
Use withSelect instead of withAPIData
mirka Jun 6, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion components/clipboard-button/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,17 @@ class ClipboardButton extends Component {
const classes = classnames( 'components-clipboard-button', className );
const ComponentToUse = icon ? IconButton : Button;

// Workaround for inconsistent behavior in Safari, where <textarea> is not
// 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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@nb nb May 29, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in #7106

const focusOnCopyEventTarget = ( e ) => {
e.target.focus();
};

return (
<span ref={ this.bindContainer }>
<span ref={ this.bindContainer } onCopy={ focusOnCopyEventTarget }>
<ComponentToUse { ...buttonProps } className={ classes }>
{ children }
</ComponentToUse>
Expand Down
228 changes: 228 additions & 0 deletions core-blocks/file/edit.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,228 @@
/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { getBlobByURL, revokeBlobURL } from '@wordpress/utils';
import {
ClipboardButton,
IconButton,
Toolbar,
withAPIData,
withNotices,
} from '@wordpress/components';
import { Component, compose, Fragment } from '@wordpress/element';
import {
MediaUpload,
MediaPlaceholder,
BlockControls,
RichText,
editorMediaUpload,
} from '@wordpress/editor';

/**
* Internal dependencies
*/
import './editor.scss';
import FileBlockInspector from './inspector';
import FileBlockEditableLink from './editable-link';

class FileEdit extends Component {
constructor() {
super( ...arguments );

const {
showDownloadButton = true,
buttonText = __( 'Download' ),
} = this.props.attributes;

this.onSelectFile = this.onSelectFile.bind( this );

// Initialize default values if undefined
this.props.setAttributes( {
showDownloadButton,
buttonText,
} );

this.state = {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

showCopyConfirmation: false,
};
}

componentDidMount() {
const { href } = this.props.attributes;

// Upload a file drag-and-dropped into the editor
if ( this.isBlobURL( href ) ) {
getBlobByURL( href )
.then( ( file ) => {
editorMediaUpload( {
allowedType: '*',
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

filesList: [ file ],
onFileChange: ( [ media ] ) => this.onSelectFile( media ),
} );
revokeBlobURL( href );
} );
}
}

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

Choose a reason for hiding this comment

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

This worked great for me as a user.

this.setState( { showCopyConfirmation: false } );
}
}

onSelectFile( media ) {
if ( media && media.url ) {
this.props.setAttributes( {
href: media.url,
fileName: media.title,
textLinkHref: media.url,
id: media.id,
}, this.props.media.get /* refetch attachment page url */ );
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

}
}

isBlobURL( url = '' ) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

return url.indexOf( 'blob:' ) === 0;
}

render() {
const {
fileName,
href,
textLinkHref,
openInNewWindow,
showDownloadButton,
buttonText,
id,
} = this.props.attributes;
const {
className,
isSelected,
setAttributes,
noticeUI,
noticeOperations,
media,
} = this.props;
const { showCopyConfirmation } = this.state;
const attachmentPage = ( id !== undefined ) && media.data && media.data.link;

const classNames = [
className,
this.isBlobURL( href ) ? 'is-transient' : '',
].join( ' ' );

const confirmCopyURL = () => {
this.setState( { showCopyConfirmation: true } );
};
const resetCopyConfirmation = () => {
this.setState( { showCopyConfirmation: false } );
};

// Choose Media File or Attachment Page (when file is in Media Library)
const changeLinkDestinationOption = ( newHref ) => {
setAttributes( { textLinkHref: newHref } );
};
const changeOpenInNewWindow = ( newValue ) => {
setAttributes( {
openInNewWindow: newValue ? '_blank' : false,
} );
};
const changeShowDownloadButton = ( newValue ) => {
setAttributes( { showDownloadButton: newValue } );
};

if ( ! href ) {
return (
<MediaPlaceholder
icon="media-default"
labels={ {
title: __( 'File' ),
name: __( 'a file' ),
} }
onSelect={ this.onSelectFile }
notices={ noticeUI }
onError={ noticeOperations.createErrorNotice }
accept="*"
type="*"
/>
);
}

return (
<Fragment>
<FileBlockInspector
hrefs={ { href, textLinkHref, attachmentPage } }
{ ...{
openInNewWindow,
showDownloadButton,
changeLinkDestinationOption,
changeOpenInNewWindow,
changeShowDownloadButton,
} }
/>
<BlockControls>
<Toolbar>
<MediaUpload
onSelect={ this.onSelectFile }
type="*"
value={ id }
render={ ( { open } ) => (
<IconButton
className="components-toolbar__control"
label={ __( 'Edit file' ) }
onClick={ open }
icon="edit"
/>
) }
/>
</Toolbar>
</BlockControls>
<div className={ classNames }>
<div>
<FileBlockEditableLink
className={ className }
placeholder={ __( 'Write file name…' ) }
text={ fileName }
href={ textLinkHref }
updateFileName={ ( text ) => setAttributes( { fileName: text } ) }
/>
{ showDownloadButton &&
<div className={ `${ className }__button-richtext-wrapper` }>
<RichText
tagName="div" // must be block-level or else cursor disappears
className={ `${ className }__button` }
value={ buttonText }
formattingControls={ [] } // disable controls
placeholder={ __( 'Add text…' ) }
keepPlaceholderOnFocus
multiline="false"
onChange={ ( text ) => setAttributes( { buttonText: text } ) }
/>
</div>
}
</div>
{ isSelected &&
<ClipboardButton
isDefault
text={ href }
className={ `${ className }__copy-url-button` }
onCopy={ confirmCopyURL }
onFinishCopy={ resetCopyConfirmation }
>
{ showCopyConfirmation ? __( 'Copied!' ) : __( 'Copy URL' ) }
</ClipboardButton>
}
</div>
</Fragment>
);
}
}

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

@gziolo gziolo Jun 5, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 );

Copy link
Member Author

Choose a reason for hiding this comment

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

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

} ) ),
withNotices,
] )( FileEdit );
81 changes: 81 additions & 0 deletions core-blocks/file/editable-link.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/**
* WordPress dependencies
*/
import { Component, Fragment } from '@wordpress/element';

export default class FileBlockEditableLink extends Component {
constructor() {
super( ...arguments );

this.state = {
showPlaceholder: ! this.props.text,
};
}

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

}
}

render() {
const { className, placeholder, text, href, updateFileName } = this.props;
const { showPlaceholder } = this.state;

const copyLinkToClipboard = ( e ) => {
const selectedText = document.getSelection().toString();
const htmlLink = `<a href="${ href }">${ selectedText }</a>`;
e.clipboardData.setData( 'text/plain', selectedText );
e.clipboardData.setData( 'text/html', htmlLink );
};

const forcePlainTextPaste = ( e ) => {
e.preventDefault();

const selection = document.getSelection();
const clipboard =
e.clipboardData.getData( 'text/plain' ).replace( /[\n\r]/g, '' );
const textNode = document.createTextNode( clipboard );

selection.getRangeAt( 0 ).insertNode( textNode );
selection.collapseToEnd();
};

const showPlaceholderIfEmptyString = ( e ) => {
if ( e.target.innerText === '' ) {
this.setState( { showPlaceholder: true } );
} else {
this.setState( { showPlaceholder: false } );
}
};

return (
<Fragment>
<a
aria-label={ placeholder }
className={ `${ className }__textlink` }
contentEditable={ true }
href={ href }
onBlur={ ( e ) => updateFileName( e.target.innerText ) }
onInput={ showPlaceholderIfEmptyString }
onCopy={ copyLinkToClipboard }
onCut={ copyLinkToClipboard }
onPaste={ forcePlainTextPaste }
>
{ text }
</a>
{ showPlaceholder &&
// Disable reason: Only useful for mouse users
/* eslint-disable jsx-a11y/no-static-element-interactions, jsx-a11y/click-events-have-key-events */
<span
className={ `${ className }__textlink-placeholder` }
onClick={ ( e ) => e.target.previousSibling.focus() }
>
{ placeholder }
</span>
/* eslint-enable jsx-a11y/no-static-element-interactions, jsx-a11y/click-events-have-key-events */
}
</Fragment>
);
}
}
35 changes: 35 additions & 0 deletions core-blocks/file/editor.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
.wp-block-file {
display: flex;
justify-content: space-between;
align-items: center;
margin-bottom: 0;

&.is-transient {
@include loading_fade;
}

&__textlink {
color: $blue-medium-700;
min-width: 1em;
text-decoration: underline;

&:focus {
box-shadow: none;
color: $blue-medium-700;
}
}

&__textlink-placeholder {
opacity: .5;
text-decoration: underline;
}

&__button-richtext-wrapper {
display: inline-block;
margin-left: 0.75em;
}

&__copy-url-button {
margin-left: 1em;
}
}
Loading