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

Recent blocks #1694

Merged
merged 5 commits into from
Jul 11, 2017
Merged

Recent blocks #1694

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8f00652
Simple "recent blocks" implementation
notnownikki Jul 4, 2017
ead5309
Fixed up tests, removed ordering recent blocks by title
notnownikki Jul 11, 2017
e455346
Removed redundant search method, clarified some other method names
notnownikki Jul 11, 2017
9286b68
Remove focus search on switching tab
notnownikki Jul 11, 2017
1ef4308
Fix dependency declaration comment
notnownikki Jul 11, 2017
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
106 changes: 69 additions & 37 deletions editor/inserter/menu.js
View file
Open in desktop
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,13 @@ 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.applySearchFilter = this.applySearchFilter.bind( this );
this.getBlocksForCurrentTab = this.getBlocksForCurrentTab.bind( this );
this.sortBlocks = this.sortBlocks.bind( this );
this.addRecentBlocks = this.addRecentBlocks.bind( this );
this.filterRecentForSearch = this.filterRecentForSearch.bind( this );
}

componentDidMount() {
Expand All @@ -44,10 +47,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 @@ -58,37 +57,69 @@ class InserterMenu extends Component {
} );
}

selectBlock( name ) {
selectBlock( blockType ) {
Copy link
Member

Choose a reason for hiding this comment

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

Why pass blockType if only using the name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we used to store the whole block before that data was moved into the app state. Will change!

return () => {
this.props.onSelect( name );
this.props.onSelect( blockType.name );
this.setState( {
filterValue: '',
currentFocus: null,
} );
};
}

getVisibleBlocks( blockTypes ) {
return filter( blockTypes, this.isShownBlock );
applySearchFilter( blockTypes ) {
Copy link
Member

Choose a reason for hiding this comment

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

This could be just be searchBlocks.

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

filterRecentForSearch( blocksByCategory ) {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I understand why we need this function. Wouldn't search just search all available blocks and list the results?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I guess this is made redundant by the recent "search searches everything" change. Will remove.

blocksByCategory.recent = this.applySearchFilter( blocksByCategory.recent );
return blocksByCategory;
}

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

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

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

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

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

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

// If the block list is empty return early.
if ( ! sortedByCategory.length ) {
Expand Down Expand Up @@ -231,7 +262,7 @@ class InserterMenu extends Component {
role="menuitem"
key={ block.name }
className="editor-inserter__block"
onClick={ this.selectBlock( block.name ) }
onClick={ this.selectBlock( block ) }
ref={ this.bindReferenceNode( block.name ) }
tabIndex="-1"
onMouseEnter={ this.props.showInsertionPoint }
Expand All @@ -245,12 +276,13 @@ class InserterMenu extends Component {

switchTab( tab ) {
this.setState( { tab: tab } );
this.setSearchFocus();
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this? Sounds like it could mess with accessibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

this was to try and make it easier to use for people who are mostly keyboard based. There's no way to switch between tabs with the keyboard right now, so to make it easier to find the block you want, I put the focus into search so you could either start typing the name of a block or start tabbing through the list without any further mouse clicks. Happy to take it out, but it saved a lot of mouse movement when testing search and tabbing!

Copy link
Member Author

Choose a reason for hiding this comment

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

There again, that was before search was at the top. I'll leave the call to someone who knows better!

Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this for now to not mess with focus. We'll need another pass of keyboard flows here once things settle.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

}

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 +304,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 +403,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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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