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

Improve reusable block creation flow #5428

Merged
merged 4 commits into from Mar 12, 2018

Conversation

Projects
None yet
2 participants
@noisysocks
Copy link
Member

noisysocks commented Mar 6, 2018

Description

Closes #4527.

Modifies the flow to create a reusable block to look like this:

  1. The user selects a block and clicks Convert to Reusable Block.

screen shot 2018-02-28 at 10 28 02

  1. The reusable block is created and is immediately in editing mode. The title of the block is selected, allowing the user to start typing the title straight away. If the user abandons the flow at this point, the new block will be titled Untitled block.

screen shot 2018-02-28 at 10 28 18

  1. The user clicks Save to rename their reusable block.

screen shot 2018-02-28 at 10 28 41

This removes two steps from the flow of creating a reusable block.

Testing

  1. Create a block
  2. Convert it to a reusable block. Confirm that the title input is selected straight away
  3. Test creating and inserting reusable blocks
Improve reusable block creation flow
When a reusable block is created, immediately put it into editing mode
and select the title <input>. This reduces the amount of steps required
to create a reusable block.

@noisysocks noisysocks requested review from aduth and WordPress/gutenberg-core and removed request for aduth Mar 6, 2018

@aduth

aduth approved these changes Mar 12, 2018

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

/**

This comment has been minimized.

@aduth

aduth Mar 12, 2018

Member

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.

This comment has been minimized.

@noisysocks

noisysocks Mar 12, 2018

Author Member

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.

this.titleRef = ref;
}

componentDidMount() {

This comment has been minimized.

@aduth

aduth Mar 12, 2018

Member

Conventionally like to see all component lifecycle following the constructor, with no functions in between.

This comment has been minimized.

@noisysocks

noisysocks Mar 12, 2018

Author Member

👌 I've moved it in c7e444f.

@@ -27,8 +27,10 @@ class ReusableBlockEdit extends Component {
this.setTitle = this.setTitle.bind( this );
this.updateReusableBlock = this.updateReusableBlock.bind( this );

const { reusableBlock } = this.props;

This comment has been minimized.

@aduth

aduth Mar 12, 2018

Member

I'd thought this not to work in constructor, since props is otherwise passed as the first argument of the constructor function. Not sure if there's a strong consensus on whether this should be discouraged though.

This comment has been minimized.

@noisysocks

noisysocks Mar 12, 2018

Author Member

If nothing else, it's simpler! Fixed in 5af814e.

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

This comment has been minimized.

@aduth

aduth Mar 12, 2018

Member

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

This comment has been minimized.

@noisysocks

noisysocks Mar 12, 2018

Author Member

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.

noisysocks added some commits Mar 12, 2018

Re-arrange ReusableBlockEditPanel methods
Move bindTitleRef so that the React lifecycle methods immediately follow
the constructor.
Don't access this.props in the ReusableBlockEdit constructor
There's no need, as we're passed props as the constructor's first
argument.
Eliminate unnecessary <span>
Simplify the edit panel DOM tree by replacing <span><b /></span> with
just the single <b>.

@noisysocks noisysocks merged commit 0f5cb93 into master Mar 12, 2018

2 checks passed

codecov/project 40.54% (+0.92%) compared to 77465b5
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@noisysocks noisysocks deleted the update/reusable-block-flow branch Mar 12, 2018

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