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.

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

Improve reusable block creation flow #5428

Merged
merged 4 commits into from
Mar 12, 2018
+91 −62
Merged

Improve reusable block creation flow #5428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d704486
Improve reusable block creation flow
noisysocks Mar 6, 2018
c7e444f
Re-arrange ReusableBlockEditPanel methods
noisysocks Mar 12, 2018
5af814e
Don't access this.props in the ReusableBlockEdit constructor
noisysocks Mar 12, 2018
3b92ad5
Eliminate unnecessary <span>
noisysocks Mar 12, 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
146 changes: 89 additions & 57 deletions blocks/library/block/edit-panel/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* WordPress dependencies
*/
import { Button } from '@wordpress/components';
import { Component, Fragment } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { keycodes } from '@wordpress/utils';

Expand All @@ -10,66 +11,97 @@ import { keycodes } from '@wordpress/utils';
*/
import './style.scss';

/**
Copy link
Member

Choose a reason for hiding this comment

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

FWIW I don't think it's so bad in this case, because it's fairly obvious what ESCAPE is, but I dislike the idea of a "Module constants" docblock in general, because it normalizes the idea that we don't need to document variables we define, and can instead apply a blanket grouping to them.

Copy link
Member Author

@noisysocks noisysocks Mar 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I'll leave this for now since we do this in a few places and I'd prefer we're somewhat consistent. It might be worth going through and auditing our usage of this pattern, since you raise a really good point.

* Module constants
*/
const { ESCAPE } = keycodes;

function ReusableBlockEditPanel( props ) {
const { isEditing, title, isSaving, onEdit, onChangeTitle, onSave, onCancel } = props;
class ReusableBlockEditPanel extends Component {
constructor() {
super( ...arguments );

this.bindTitleRef = this.bindTitleRef.bind( this );
this.handleFormSubmit = this.handleFormSubmit.bind( this );
this.handleTitleChange = this.handleTitleChange.bind( this );
this.handleTitleKeyDown = this.handleTitleKeyDown.bind( this );
}

componentDidMount() {
if ( this.props.isEditing ) {
this.titleRef.select();
}
}

bindTitleRef( ref ) {
this.titleRef = ref;
}

return [
( ! isEditing && ! isSaving ) && (
<div key="view" className="reusable-block-edit-panel">
<span className="reusable-block-edit-panel__info">
<b>{ title }</b>
Copy link
Member

Choose a reason for hiding this comment

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

Of course not relevant to this pull request, but was <b> chosen for a particular reason?

Do not confuse the element with the , , or elements. The element represents text of certain importance, puts some emphasis on the text and the element represents text of certain relevance. The element doesn't convey such special semantic information; use it only when no others fit.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/b

Further, do we need both the span and the b, or could we collapse to eliminate one or the other (probably just replacing span with desired inline tag).

Copy link
Member Author

@noisysocks noisysocks Mar 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Eliminated the unnecessary <span> in 3b92ad5. I like the <b> in lieu of a <span> since it denotes that there's something special (well, bold...) about this text to the developer who is reading the JSX.

</span>
<Button
isLarge
className="reusable-block-edit-panel__button"
onClick={ onEdit }>
{ __( 'Edit' ) }
</Button>
</div>
),
( isEditing || isSaving ) && (
<form
key="edit"
className="reusable-block-edit-panel"
onSubmit={ ( event ) => {
event.preventDefault();
onSave();
} }>
<input
type="text"
disabled={ isSaving }
className="reusable-block-edit-panel__title"
value={ title }
onChange={ ( event ) => onChangeTitle( event.target.value ) }
onKeyDown={ ( event ) => {
if ( event.keyCode === ESCAPE ) {
event.stopPropagation();
onCancel();
}
} } />
<Button
type="submit"
isPrimary
isLarge
isBusy={ isSaving }
disabled={ ! title || isSaving }
className="reusable-block-edit-panel__button"
onClick={ onSave }>
{ __( 'Save' ) }
</Button>
<Button
isLarge
disabled={ isSaving }
className="reusable-block-edit-panel__button"
onClick={ onCancel }>
{ __( 'Cancel' ) }
</Button>
</form>
),
];
handleFormSubmit( event ) {
event.preventDefault();
this.props.onSave();
}

handleTitleChange( event ) {
this.props.onChangeTitle( event.target.value );
}

handleTitleKeyDown( event ) {
if ( event.keyCode === ESCAPE ) {
event.stopPropagation();
this.props.onCancel();
}
}

render() {
const { isEditing, title, isSaving, onEdit, onSave, onCancel } = this.props;

return (
<Fragment>
{ ( ! isEditing && ! isSaving ) && (
<div className="reusable-block-edit-panel">
<b className="reusable-block-edit-panel__info">
{ title }
</b>
<Button isLarge className="reusable-block-edit-panel__button" onClick={ onEdit }>
{ __( 'Edit' ) }
</Button>
</div>
) }
{ ( isEditing || isSaving ) && (
<form className="reusable-block-edit-panel" onSubmit={ this.handleFormSubmit }>
<input
ref={ this.bindTitleRef }
type="text"
disabled={ isSaving }
className="reusable-block-edit-panel__title"
value={ title }
onChange={ this.handleTitleChange }
onKeyDown={ this.handleTitleKeyDown }
/>
<Button
type="submit"
isPrimary
isLarge
isBusy={ isSaving }
disabled={ ! title || isSaving }
className="reusable-block-edit-panel__button"
onClick={ onSave }
>
{ __( 'Save' ) }
</Button>
<Button
isLarge
disabled={ isSaving }
className="reusable-block-edit-panel__button"
onClick={ onCancel }
>
{ __( 'Cancel' ) }
</Button>
</form>
) }
</Fragment>
);
}
}

export default ReusableBlockEditPanel;

7 changes: 2 additions & 5 deletions blocks/library/block/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import BlockEdit from '../../block-edit';
import ReusableBlockEditPanel from './edit-panel';

class ReusableBlockEdit extends Component {
constructor() {
constructor( { reusableBlock } ) {
super( ...arguments );

this.startEditing = this.startEditing.bind( this );
Expand All @@ -28,7 +28,7 @@ class ReusableBlockEdit extends Component {
this.updateReusableBlock = this.updateReusableBlock.bind( this );

this.state = {
isEditing: false,
isEditing: !! ( reusableBlock && reusableBlock.isTemporary ),
title: null,
attributes: null,
};
Expand All @@ -40,9 +40,6 @@ class ReusableBlockEdit extends Component {
}
}

/**
* @inheritdoc
*/
componentWillReceiveProps( nextProps ) {
if ( this.props.focus && ! nextProps.focus ) {
this.stopEditing();
Expand Down