Skip to content

Framework: Drop the focus/setFocus props from block edit functions #4872

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.

Sign up for GitHub

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

Merged
youknowriad merged 11 commits into from
Feb 7, 2018
Merged

Framework: Drop the focus/setFocus props from block edit functions #4872

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
922cb6c
Framework: Drop the focus/setFocus props from block edit functions
youknowriad Feb 5, 2018
ccd42a1
Fix multiple editable blocks (switching between editables)
youknowriad Feb 5, 2018
7fc8cb5
Show the toolbar when the selection is not collapsed
youknowriad Feb 6, 2018
18b75b5
Small tweaks
youknowriad Feb 6, 2018
2645710
Fix small bug when the selection is empty
youknowriad Feb 6, 2018
280e1b8
Select a block when moving the cursor
youknowriad Feb 6, 2018
98e6026
Stop propagating backspace and enter keys from the URL input
youknowriad Feb 6, 2018
78891a3
Fix small bug when reaching the end of the editor
youknowriad Feb 6, 2018
6b3bf14
Clarify the caret position
youknowriad Feb 7, 2018
83c42e5
Update Gallery Block after rebase
youknowriad Feb 7, 2018
80d0a4c
Fix e2e tests
youknowriad Feb 7, 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
Next Next commit
Framework: Drop the focus/setFocus props from block edit functions
  • Loading branch information
@youknowriad
youknowriad committed Feb 7, 2018
commit 922cb6cafb9663b61362251823d1d883252211b4
8 changes: 4 additions & 4 deletions blocks/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,9 +277,9 @@ want to display alignment options in the selected block's toolbar.

Because the toolbar should only be shown when the block is selected, it is
important that a `BlockControls` element is only returned when the block's
`focus` prop is
`isSelected` prop is
[truthy](https://developer.mozilla.org/en-US/docs/Glossary/Truthy),
meaning that focus is currently within the block.
meaning that the block is currently selected.

Example:

Expand All @@ -291,8 +291,8 @@ Example:

function edit( props ) {
return [
// Controls: (only visible when focused)
props.focus && (
// Controls: (only visible when block is selected)
props.isSelected && (
el( BlockControls, { key: 'controls' },
el( AlignmentToolbar, {
value: props.align,
Expand Down
12 changes: 11 additions & 1 deletion blocks/block-edit/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* External dependencies
*/
import classnames from 'classnames';
import { noop } from 'lodash';

/**
* WordPress dependencies
Expand Down Expand Up @@ -36,7 +37,16 @@ export function BlockEdit( props ) {
// them preferencially as the render value for the block.
const Edit = blockType.edit || blockType.save;

return <Edit { ...props } className={ className } />;
// For backwards compatibility concerns adds a focus and setFocus prop
// These should be removed after some time (maybe when merging to Core)
Copy link
Contributor

@mcsf mcsf Feb 6, 2018

Choose a reason for hiding this comment

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

Do you think it's worth looking for availability of window.Proxy and, if so, return an instance of it?

focus = new Proxy( {}, { get() { console.warn( 'deprecated' ) } } )
return <Edit … focus={ focus } />
> focus
< Proxy { <target>: {}, <handler>: {…} }

> focus.editable
⚠ deprecated
< undefined

Copy link
Contributor Author

@youknowriad youknowriad Feb 6, 2018

Choose a reason for hiding this comment

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

I like it 👍

Copy link
Contributor Author

@youknowriad youknowriad Feb 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

On a second thought, we'd have to proxify the props object to do so, which I think is not really possible :(

Copy link
Contributor Author

@youknowriad youknowriad Feb 6, 2018

Choose a reason for hiding this comment

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

Well I guess it's fine, it seems supported where we need it.

Copy link
Contributor

@mcsf mcsf Feb 6, 2018

Choose a reason for hiding this comment

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

we'd have to proxify the props object

What do you mean by this? I was just thinking of the following pseudo-code:

if not isSelected: focus = false
else:
  if browserHasProxySupport: focus = deprecatedEmptyObject
  else: focus = {}

Copy link
Contributor Author

@youknowriad youknowriad Feb 6, 2018

Choose a reason for hiding this comment

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

I was trying to find a way to proxy any access to props.focus (and not focus.*) because the main use-case for this prop is just checking truthiness to show the block toolbar/inspector

Copy link
Contributor

@mcsf mcsf Feb 7, 2018

Choose a reason for hiding this comment

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

Gotcha.

return (
<Edit
{ ...props }
className={ className }
focus={ props.isSelected ? {} : false }
setFocus={ noop }
/>
);
}

export default withFilters( 'blocks.BlockEdit' )( BlockEdit );
10 changes: 3 additions & 7 deletions blocks/library/audio/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ export const settings = {
}
render() {
const { align, caption, id } = this.props.attributes;
const { setAttributes, focus, setFocus } = this.props;
const { setAttributes, isSelected } = this.props;
const { editing, className, src } = this.state;
const updateAlignment = ( nextAlign ) => setAttributes( { align: nextAlign } );
const switchToEditing = () => {
Expand All @@ -93,7 +93,7 @@ export const settings = {
}
return false;
};
const controls = focus && (
const controls = isSelected && (
<BlockControls key="controls">
<BlockAlignmentToolbar
value={ align }
Expand All @@ -110,8 +110,6 @@ export const settings = {
</BlockControls>
);

const focusCaption = ( focusValue ) => setFocus( { editable: 'caption', ...focusValue } );

if ( editing ) {
return [
controls,
Expand Down Expand Up @@ -153,13 +151,11 @@ export const settings = {
controls,
<figure key="audio" className={ className }>
<audio controls="controls" src={ src } />
{ ( ( caption && caption.length ) || !! focus ) && (
{ ( ( caption && caption.length ) || !! isSelected ) && (
<RichText
tagName="figcaption"
placeholder={ __( 'Write caption…' ) }
value={ caption }
focus={ focus && focus.editable === 'caption' ? focus : undefined }
onFocus={ focusCaption }
onChange={ ( value ) => setAttributes( { caption: value } ) }
inlineToolbar
/>
Expand Down
6 changes: 3 additions & 3 deletions blocks/library/block/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ class ReusableBlockEdit extends Component {
}

render() {
const { focus, reusableBlock, isFetching, isSaving } = this.props;
const { isSelected, reusableBlock, isFetching, isSaving } = this.props;
const { isEditing, title, attributes } = this.state;

if ( ! reusableBlock && isFetching ) {
Expand All @@ -105,12 +105,12 @@ class ReusableBlockEdit extends Component {
<BlockEdit
{ ...this.props }
name={ reusableBlock.type }
focus={ isEditing ? focus : null }
isSelected={ isEditing ? isSelected : false }
Copy link
Contributor

@mcsf mcsf Feb 6, 2018

Choose a reason for hiding this comment

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

Totally minor, but now that we only deal with booleans we can simplify as { isEditing && isSelected }

attributes={ reusableBlockAttributes }
setAttributes={ isEditing ? this.setAttributes : noop }
/>
</div>,
focus && (
isSelected && (
<ReusableBlockEditPanel
key="panel"
isEditing={ isEditing }
Expand Down
11 changes: 4 additions & 7 deletions blocks/library/button/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,7 @@ class ButtonBlock extends Component {
const {
attributes,
setAttributes,
focus,
setFocus,
isSelected,
className,
} = this.props;

Expand All @@ -76,7 +75,7 @@ class ButtonBlock extends Component {
} = attributes;

return [
focus && (
isSelected && (
<BlockControls key="controls">
<BlockAlignmentToolbar value={ align } onChange={ this.updateAlignment } />
</BlockControls>
Expand All @@ -86,8 +85,6 @@ class ButtonBlock extends Component {
tagName="span"
placeholder={ __( 'Add text…' ) }
value={ text }
focus={ focus }
onFocus={ setFocus }
onChange={ ( value ) => setAttributes( { text: value } ) }
formattingControls={ [ 'bold', 'italic', 'strikethrough' ] }
className="wp-block-button__link"
Expand All @@ -97,7 +94,7 @@ class ButtonBlock extends Component {
} }
keepPlaceholderOnFocus
/>
{ focus &&
{ isSelected &&
<InspectorControls key="inspector">
<ToggleControl
label={ __( 'Wrap text' ) }
Expand Down Expand Up @@ -125,7 +122,7 @@ class ButtonBlock extends Component {
</InspectorControls>
}
</span>,
focus && (
isSelected && (
<form
key="form-link"
className="blocks-button__inline-link"
Expand Down
10 changes: 3 additions & 7 deletions blocks/library/cover-image/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ export const settings = {
}
},

edit( { attributes, setAttributes, focus, setFocus, className } ) {
edit( { attributes, setAttributes, isSelected, className } ) {
const { url, title, align, contentAlign, id, hasParallax, dimRatio } = attributes;
const updateAlignment = ( nextAlign ) => setAttributes( { align: nextAlign } );
const onSelectImage = ( media ) => setAttributes( { url: media.url, id: media.id } );
Expand Down Expand Up @@ -124,7 +124,7 @@ export const settings = {
} }
/>
);
const controls = focus && [
const controls = isSelected && [
<BlockControls key="controls">
<BlockAlignmentToolbar
value={ align }
Expand Down Expand Up @@ -176,8 +176,6 @@ export const settings = {
<RichText
tagName="h2"
value={ title }
focus={ focus }
onFocus={ setFocus }
onChange={ ( value ) => setAttributes( { title: value } ) }
inlineToolbar
/>
Expand All @@ -199,13 +197,11 @@ export const settings = {
style={ style }
className={ classes }
>
{ title || !! focus ? (
{ title || isSelected ? (
<RichText
tagName="h2"
placeholder={ __( 'Write title…' ) }
value={ title }
focus={ focus }
onFocus={ setFocus }
onChange={ ( value ) => setAttributes( { title: value } ) }
inlineToolbar
/>
Expand Down
9 changes: 3 additions & 6 deletions blocks/library/embed/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,10 @@ function getEmbedBlockSettings( { title, icon, category = 'embed', transforms, k
render() {
const { html, type, error, fetching } = this.state;
const { align, url, caption } = this.props.attributes;
const { setAttributes, focus, setFocus } = this.props;
const { setAttributes, isSelected } = this.props;
const updateAlignment = ( nextAlign ) => setAttributes( { align: nextAlign } );

const controls = focus && (
const controls = isSelected && (
<BlockControls key="controls">
<BlockAlignmentToolbar
value={ align }
Expand Down Expand Up @@ -200,17 +200,14 @@ function getEmbedBlockSettings( { title, icon, category = 'embed', transforms, k
html={ html }
title={ iframeTitle }
type={ type }
onFocus={ () => setFocus() }
/>
</div>
) }
{ ( caption && caption.length > 0 ) || !! focus ? (
{ ( caption && caption.length > 0 ) || isSelected ? (
<RichText
tagName="figcaption"
placeholder={ __( 'Write caption…' ) }
value={ caption }
focus={ focus }
onFocus={ setFocus }
Copy link
Member

@aduth aduth Feb 9, 2018

Choose a reason for hiding this comment

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

This change regressed on the fix introduced by #4011, where in that case I explicitly wanted to handle focus within the iframe. Can we otherwise expose manually setting a block as selected?

Copy link
Contributor Author

@youknowriad youknowriad Feb 9, 2018

Choose a reason for hiding this comment

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

You mean like a onSelect prop or something? I'd prefer to avoid it in favor of a global event but if it's not possible for iframes, we could provide it

Copy link
Member

@aduth aduth Feb 9, 2018

Choose a reason for hiding this comment

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

We could see about generalizing the blur check which occurs in Sandbox, to see if focus has transitioned to an iframe within a block:

gutenberg/components/sandbox/index.js

Lines 54 to 73 in 98d6c7e

componentDidMount() {
window.addEventListener( 'message', this.checkMessageForResize, false );
window.addEventListener( 'blur', this.checkFocus );
this.trySandbox();
}
componentDidUpdate() {
this.trySandbox();
}
componentWillUnmount() {
window.removeEventListener( 'message', this.checkMessageForResize );
window.removeEventListener( 'blur', this.checkFocus );
}
checkFocus() {
if ( this.props.onFocus && document.activeElement === this.iframe ) {
this.props.onFocus();
}
}

onChange={ ( value ) => setAttributes( { caption: value } ) }
inlineToolbar
/>
Expand Down
12 changes: 2 additions & 10 deletions blocks/library/freeform/old-editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,6 @@ export default class OldEditor extends Component {
if ( prevProps.attributes.content !== content ) {
editor.setContent( content || '' );
}

if ( ! prevProps.focus && !! this.props.focus && document.activeElement !== editor.getBody() ) {
editor.getBody().focus();
}
}

initialize() {
Expand Down Expand Up @@ -127,10 +123,6 @@ export default class OldEditor extends Component {
// See wp-includes/js/tinymce/plugins/wordpress/plugin.js
// Swaps node.nodeName === 'BODY' to node === editor.getBody()
editor.on( 'init', () => {
if ( this.props.focus && document.activeElement !== editor.getBody() ) {
editor.getBody().focus();
}

editor.addCommand( 'WP_More', function( tag ) {
var parent, html, title,
classname = 'wp-more-tag',
Expand Down Expand Up @@ -176,15 +168,15 @@ export default class OldEditor extends Component {
}

render() {
const { focus, id, className } = this.props;
const { isSelected, id, className } = this.props;

return [
<div
key="toolbar"
id={ id + '-toolbar' }
ref={ ref => this.ref = ref }
className="freeform-toolbar"
style={ ! focus ? { display: 'none' } : {} }
style={ ! isSelected ? { display: 'none' } : {} }
/>,
<div
key="editor"
Expand Down
10 changes: 5 additions & 5 deletions blocks/library/gallery/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,16 +145,16 @@ class GalleryBlock extends Component {
}

componentWillReceiveProps( nextProps ) {
// Deselect images when losing focus
if ( ! nextProps.focus && this.props.focus ) {
// Deselect images when deselecting the block
if ( ! nextProps.isSelected && this.props.isSelected ) {
this.setState( {
selectedImage: null,
} );
}
}

render() {
const { attributes, focus, className } = this.props;
const { attributes, isSelected, className } = this.props;
const { images, columns = defaultColumnsNumber( attributes ), align, imageCrop, linkTo } = attributes;

const dropZone = (
Expand All @@ -164,7 +164,7 @@ class GalleryBlock extends Component {
);

const controls = (
focus && (
isSelected && (
<BlockControls key="controls">
<BlockAlignmentToolbar
value={ align }
Expand Down Expand Up @@ -208,7 +208,7 @@ class GalleryBlock extends Component {

return [
controls,
focus && (
isSelected && (
<InspectorControls key="inspector">
<h2>{ __( 'Gallery Settings' ) }</h2>
{ images.length > 1 && <RangeControl
Expand Down
8 changes: 3 additions & 5 deletions blocks/library/heading/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,11 @@ export const settings = {
};
},

edit( { attributes, setAttributes, focus, setFocus, mergeBlocks, insertBlocksAfter, onReplace } ) {
edit( { attributes, setAttributes, isSelected, mergeBlocks, insertBlocksAfter, onReplace } ) {
const { align, content, nodeName, placeholder } = attributes;

return [
focus && (
isSelected && (
<BlockControls
key="controls"
controls={
Expand All @@ -119,7 +119,7 @@ export const settings = {
}
/>
),
focus && (
isSelected && (
<InspectorControls key="inspector">
<h3>{ __( 'Heading Settings' ) }</h3>
<p>{ __( 'Level' ) }</p>
Expand Down Expand Up @@ -148,8 +148,6 @@ export const settings = {
wrapperClassName="wp-block-heading"
tagName={ nodeName.toLowerCase() }
value={ content }
focus={ focus }
onFocus={ setFocus }
onChange={ ( value ) => setAttributes( { content: value } ) }
onMerge={ mergeBlocks }
onSplit={
Expand Down
4 changes: 2 additions & 2 deletions blocks/library/html/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ export const settings = {

edit: withState( {
preview: false,
} )( ( { attributes, setAttributes, setState, focus, preview } ) => [
focus && (
} )( ( { attributes, setAttributes, setState, isSelected, preview } ) => [
isSelected && (
<BlockControls key="controls">
<div className="components-toolbar">
<button
Expand Down
Loading