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

Recent blocks #1694

Merged
merged 5 commits into from
Jul 11, 2017
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
92 changes: 58 additions & 34 deletions editor/inserter/menu.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { getCategories, getBlockTypes } from 'blocks';
*/
import './style.scss';
import { showInsertionPoint, hideInsertionPoint } from '../actions';
import { getRecentlyUsedBlocks } from '../selectors';

class InserterMenu extends Component {
constructor() {
Expand All @@ -29,11 +30,12 @@ class InserterMenu extends Component {
tab: 'recent',
};
this.filter = this.filter.bind( this );
this.isShownBlock = this.isShownBlock.bind( this );
this.setSearchFocus = this.setSearchFocus.bind( this );
this.onKeyDown = this.onKeyDown.bind( this );
this.getVisibleBlocks = this.getVisibleBlocks.bind( this );
this.sortBlocksByCategory = this.sortBlocksByCategory.bind( this );
this.searchBlocks = this.searchBlocks.bind( this );
this.getBlocksForCurrentTab = this.getBlocksForCurrentTab.bind( this );
this.sortBlocks = this.sortBlocks.bind( this );
this.addRecentBlocks = this.addRecentBlocks.bind( this );
}

componentDidMount() {
Expand All @@ -44,10 +46,6 @@ class InserterMenu extends Component {
document.removeEventListener( 'keydown', this.onKeyDown );
}

isShownBlock( block ) {
return block.title.toLowerCase().indexOf( this.state.filterValue.toLowerCase() ) !== -1;
}

bindReferenceNode( nodeName ) {
return ( node ) => this.nodes[ nodeName ] = node;
}
Expand All @@ -68,27 +66,53 @@ class InserterMenu extends Component {
};
}

getVisibleBlocks( blockTypes ) {
return filter( blockTypes, this.isShownBlock );
searchBlocks( blockTypes ) {
const matchesSearch = ( block ) => block.title.toLowerCase().indexOf( this.state.filterValue.toLowerCase() ) !== -1;
return filter( blockTypes, matchesSearch );
}

getBlocksForCurrentTab() {
// if we're searching, use everything, otherwise just get the blocks visible in this tab
if ( this.state.filterValue ) {
return getBlockTypes();
}
switch ( this.state.tab ) {
case 'recent':
return this.props.recentlyUsedBlocks;
case 'blocks':
return filter( getBlockTypes(), ( block ) => block.category !== 'embed' );
case 'embeds':
return filter( getBlockTypes(), ( block ) => block.category === 'embed' );
}
}

sortBlocksByCategory( blockTypes ) {
sortBlocks( blockTypes ) {
if ( 'recent' === this.state.tab && ! this.state.filterValue ) {
return blockTypes;
}

const getCategoryIndex = ( item ) => {
return findIndex( getCategories(), ( category ) => category.slug === item.category );
};

return sortBy( blockTypes, getCategoryIndex );
}

addRecentBlocks( blocksByCategory ) {
Copy link
Member

Choose a reason for hiding this comment

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

"Add" feels a bit weird in this context. Why does it need to "add" anything? Should recent be a "category" or just returned on demand for the recent tab?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, it's "adding" .recent to the object that has the blocks groups by category. We'd need to rework the render method a bit to get rid of this, which I'm happy to do, it's just this PR was getting big enough as it was :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to clarify why this needs a separate step, 'recent' isn't a category because we don't have the ability for a block to be in two categories, so the method that sorts blocks by category has no knowledge of 'recent', and this method is adding the recent category to the object that holds the blocks grouped by category.

Copy link
Member Author

Choose a reason for hiding this comment

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

To be honest, the render method could use cleaning up, there's a lot of duplication and we could simplify it a lot. Your call if you'd like this done in this PR or not, I'm happy to do it either here, or as another PR.

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 ok refactoring later.

blocksByCategory.recent = this.props.recentlyUsedBlocks;
return blocksByCategory;
}

groupByCategory( blockTypes ) {
return groupBy( blockTypes, ( blockType ) => blockType.category );
}

getVisibleBlocksByCategory( blockTypes ) {
return flow(
this.getVisibleBlocks,
this.sortBlocksByCategory,
this.groupByCategory
this.searchBlocks,
this.sortBlocks,
this.groupByCategory,
this.addRecentBlocks
)( blockTypes );
}

Expand Down Expand Up @@ -137,9 +161,9 @@ class InserterMenu extends Component {

focusNext() {
const sortedByCategory = flow(
this.getVisibleBlocks,
this.sortBlocksByCategory,
)( getBlockTypes() );
this.searchBlocks,
this.sortBlocks,
)( this.getBlocksForCurrentTab() );

// If the block list is empty return early.
if ( ! sortedByCategory.length ) {
Expand All @@ -152,9 +176,9 @@ class InserterMenu extends Component {

focusPrevious() {
const sortedByCategory = flow(
this.getVisibleBlocks,
this.sortBlocksByCategory,
)( getBlockTypes() );
this.searchBlocks,
this.sortBlocks,
)( this.getBlocksForCurrentTab() );

// If the block list is empty return early.
if ( ! sortedByCategory.length ) {
Expand Down Expand Up @@ -249,8 +273,8 @@ class InserterMenu extends Component {

render() {
const { position, instanceId } = this.props;
const visibleBlocksByCategory = this.getVisibleBlocksByCategory( getBlockTypes() );
const isSearching = this.state.filterValue;
const visibleBlocksByCategory = this.getVisibleBlocksByCategory( this.getBlocksForCurrentTab() );

/* eslint-disable jsx-a11y/no-autofocus */
return (
Expand All @@ -272,19 +296,15 @@ class InserterMenu extends Component {
<div role="menu" className="editor-inserter__content">
{ this.state.tab === 'recent' && ! isSearching &&
<div className="editor-inserter__recent">
{ getCategories()
.map( ( category ) => category.slug === 'common' && !! visibleBlocksByCategory[ category.slug ] && (
<div
className="editor-inserter__category-blocks"
role="menu"
tabIndex="0"
aria-labelledby={ `editor-inserter__separator-${ category.slug }-${ instanceId }` }
key={ category.slug }
>
{ visibleBlocksByCategory[ category.slug ].map( ( block ) => this.getBlockItem( block ) ) }
</div>
) )
}
<div
className="editor-inserter__category-blocks"
role="menu"
tabIndex="0"
aria-labelledby={ `editor-inserter__separator-${ 'recent' }-${ instanceId }` }
key={ 'recent' }
>
{ visibleBlocksByCategory.recent.map( ( block ) => this.getBlockItem( block ) ) }
</div>
</div>
}
{ this.state.tab === 'blocks' && ! isSearching &&
Expand Down Expand Up @@ -375,7 +395,11 @@ class InserterMenu extends Component {
}

const connectComponent = connect(
undefined,
( state ) => {
return {
recentlyUsedBlocks: getRecentlyUsedBlocks( state ),
};
},
{ showInsertionPoint, hideInsertionPoint }
);

Expand Down
12 changes: 12 additions & 0 deletions editor/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import createSelector from 'rememo';
/**
* Internal dependencies
*/
import { getBlockType } from 'blocks';
import { addQueryArgs } from './utils/url';

/**
Expand Down Expand Up @@ -668,3 +669,14 @@ export function getSuggestedPostFormat( state ) {
export function getNotices( state ) {
return values( state.notices );
}

/**
* Resolves the list of recently used block names into a list of block type settings.
*
* @param {Object} state Global application state
* @return {Array} List of recently used blocks
*/
export function getRecentlyUsedBlocks( state ) {
// resolves the block names in the state to the block type settings
return state.editor.recentlyUsedBlocks.map( blockType => getBlockType( blockType ) );
}
27 changes: 27 additions & 0 deletions editor/state.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ import { combineReducers, applyMiddleware, createStore } from 'redux';
import refx from 'refx';
import { reduce, keyBy, first, last, omit, without, flowRight } from 'lodash';

/**
* WordPress dependencies
*/
import { getBlockTypes } from 'blocks';
Copy link
Member

Choose a reason for hiding this comment

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

This should be internal dependency.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you clarify this? Imports from 'blocks' are WordPress dependencies in other places.

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've just checked, and everywhere else that imports things from 'blocks', it's listed as a WordPress dependency. Does everywhere else need to change, or is this specific to the getBlockTypes function? If so, can you clarify why?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, sorry, my bad, this is assuming how we'll expose these. We would need to fix this one instead: https://github.com/WordPress/gutenberg/pull/1694/files#diff-d9e543404329206d6652e45f824ae35fR11

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!


/**
* Internal dependencies
*/
Expand Down Expand Up @@ -219,6 +224,28 @@ export const editor = combineUndoableReducers( {

return state;
},

recentlyUsedBlocks( state = [], action ) {
const maxRecent = 8;
switch ( action.type ) {
case 'SETUP_NEW_POST':
// This is where we initially populate the recently used blocks,
// for now this inserts blocks from the common category.
return getBlockTypes()
.filter( ( blockType ) => 'common' === blockType.category )
.slice( 0, maxRecent )
.map( ( blockType ) => blockType.name );
case 'INSERT_BLOCKS':
// This is where we record the block usage so it can show up in
// the recent blocks.
let newState = [ ...state ];
action.blocks.forEach( ( block ) => {
newState = [ block.name, ...without( newState, block.name ) ];
} );
return newState.slice( 0, maxRecent );
}
return state;
},
}, { resetTypes: [ 'RESET_BLOCKS' ] } );

/**
Expand Down
42 changes: 41 additions & 1 deletion editor/test/state.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,11 @@ import {
describe( 'state', () => {
describe( 'editor()', () => {
beforeAll( () => {
registerBlockType( 'core/test-block', { save: noop } );
registerBlockType( 'core/test-block', {
save: noop,
edit: noop,
category: 'common',
} );
} );

afterAll( () => {
Expand Down Expand Up @@ -78,6 +82,42 @@ describe( 'state', () => {
expect( state.blockOrder ).toEqual( [ 'chicken', 'ribs' ] );
} );

it( 'should record recently used blocks', () => {
const original = editor( undefined, {} );
const state = editor( original, {
type: 'INSERT_BLOCKS',
blocks: [ {
uid: 'bacon',
name: 'core-embed/twitter',
} ],
} );

expect( state.recentlyUsedBlocks[ 0 ] ).toEqual( 'core-embed/twitter' );

const twoRecentBlocks = editor( state, {
type: 'INSERT_BLOCKS',
blocks: [ {
uid: 'eggs',
name: 'core-embed/youtube',
} ],
} );

expect( twoRecentBlocks.recentlyUsedBlocks[ 0 ] ).toEqual( 'core-embed/youtube' );
expect( twoRecentBlocks.recentlyUsedBlocks[ 1 ] ).toEqual( 'core-embed/twitter' );
} );

it( 'should populate recently used blocks with the common category', () => {
const initial = editor( undefined, {
type: 'SETUP_NEW_POST',
edits: {
status: 'draft',
title: 'post title',
},
} );

expect( initial.recentlyUsedBlocks ).toEqual( expect.arrayContaining( [ 'core/test-block', 'core/text' ] ) );
} );

it( 'should replace the block', () => {
const original = editor( undefined, {
type: 'RESET_BLOCKS',
Expand Down