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

Try: use 'frecency' to sort items in the inserters #5342

Merged
merged 2 commits into from
Mar 5, 2018
+65 −150
Merged

Try: use 'frecency' to sort items in the inserters #5342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4632d01
Use 'frecency' to sort items in the inserters
noisysocks Mar 2, 2018
f754b1a
Rename 'Recent' tab to 'Frequent'
noisysocks Mar 5, 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
4 changes: 2 additions & 2 deletions editor/components/inserter-with-shortcuts/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { __, sprintf } from '@wordpress/i18n';
*/
import './style.scss';
import Inserter from '../inserter';
import { getFrequentInserterItems } from '../../store/selectors';
import { getFrecentInserterItems } from '../../store/selectors';
import { replaceBlocks } from '../../store/actions';

function InserterWithShortcuts( { items, isLocked, onToggle, onInsert } ) {
Expand Down Expand Up @@ -62,7 +62,7 @@ export default compose(
} ),
connect(
( state, { enabledBlockTypes } ) => ( {
items: getFrequentInserterItems( state, enabledBlockTypes, 3 ),
items: getFrecentInserterItems( state, enabledBlockTypes, 3 ),
} ),
( dispatch, { uid, layout } ) => ( {
onInsert( { name, initialAttributes } ) {
Expand Down
26 changes: 13 additions & 13 deletions editor/components/inserter/menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import { keycodes } from '@wordpress/utils';
import './style.scss';
import NoBlocks from './no-blocks';

import { getInserterItems, getRecentInserterItems } from '../../store/selectors';
import { getInserterItems, getFrecentInserterItems } from '../../store/selectors';
import { fetchReusableBlocks } from '../../store/actions';
import { default as InserterGroup } from './group';
import BlockPreview from '../block-preview';
Expand All @@ -60,7 +60,7 @@ export class InserterMenu extends Component {
this.nodes = {};
this.state = {
filterValue: '',
tab: 'recent',
tab: 'frequent',
selectedItem: null,
};
this.filter = this.filter.bind( this );
Expand All @@ -69,7 +69,7 @@ export class InserterMenu extends Component {
this.sortItems = this.sortItems.bind( this );
this.selectItem = this.selectItem.bind( this );

this.tabScrollTop = { recent: 0, blocks: 0, embeds: 0 };
this.tabScrollTop = { frequent: 0, blocks: 0, embeds: 0 };
this.switchTab = this.switchTab.bind( this );
this.previewItem = this.previewItem.bind( this );
}
Expand Down Expand Up @@ -118,7 +118,7 @@ export class InserterMenu extends Component {
}

getItemsForTab( tab ) {
const { items, recentItems } = this.props;
const { items, frecentItems } = this.props;

// If we're searching, use everything, otherwise just get the items visible in this tab
if ( this.state.filterValue ) {
Expand All @@ -127,8 +127,8 @@ export class InserterMenu extends Component {

let predicate;
switch ( tab ) {
case 'recent':
return recentItems;
case 'frequent':
return frecentItems;

case 'blocks':
predicate = ( item ) => item.category !== 'embed' && item.category !== 'reusable-blocks';
Expand All @@ -147,7 +147,7 @@ export class InserterMenu extends Component {
}

sortItems( items ) {
if ( 'recent' === this.state.tab && ! this.state.filterValue ) {
if ( 'frequent' === this.state.tab && ! this.state.filterValue ) {
return items;
}

Expand Down Expand Up @@ -220,8 +220,8 @@ export class InserterMenu extends Component {
renderTabView( tab ) {
const itemsForTab = this.getItemsForTab( tab );

// If the Recent tab is selected, don't render category headers
if ( 'recent' === tab ) {
// If the Frequent tab is selected, don't render category headers
if ( 'frequent' === tab ) {
return this.renderItems( itemsForTab );
}

Expand All @@ -248,7 +248,7 @@ export class InserterMenu extends Component {

// Passed to TabbableContainer, extending its event-handling logic
eventToOffset( event ) {
// If a tab (Recent, Blocks, …) is focused, pressing the down arrow
// If a tab (Frequent, Blocks, …) is focused, pressing the down arrow
// moves focus to the selected panel below.
if (
event.keyCode === keycodes.DOWN &&
Expand Down Expand Up @@ -291,8 +291,8 @@ export class InserterMenu extends Component {
onSelect={ this.switchTab }
tabs={ [
{
name: 'recent',
title: __( 'Recent' ),
name: 'frequent',
title: __( 'Frequent' ),
className: 'editor-inserter__tab',
},
{
Expand Down Expand Up @@ -344,7 +344,7 @@ export default compose(
( state, ownProps ) => {
return {
items: getInserterItems( state, ownProps.enabledBlockTypes ),
recentItems: getRecentInserterItems( state, ownProps.enabledBlockTypes ),
frecentItems: getFrecentInserterItems( state, ownProps.enabledBlockTypes ),
};
},
{ fetchReusableBlocks }
Expand Down
24 changes: 12 additions & 12 deletions editor/components/inserter/test/menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,21 +88,21 @@ describe( 'InserterMenu', () => {
// wrapper.find have had to be strengthened (and the filterWhere strengthened also), otherwise two
// results would be returned even though only one was in the DOM.

it( 'should show the recent tab by default', () => {
it( 'should show the frequent tab by default', () => {
const wrapper = mount(
<InserterMenu
position={ 'top center' }
instanceId={ 1 }
items={ [] }
recentItems={ [] }
frecentItems={ [] }
debouncedSpeak={ noop }
fetchReusableBlocks={ noop }
blockTypes
/>
);

const activeCategory = wrapper.find( '.editor-inserter__tab button.is-active' );
expect( activeCategory.text() ).toBe( 'Recent' );
expect( activeCategory.text() ).toBe( 'Frequent' );

const visibleBlocks = wrapper.find( '.editor-inserter__block' );
expect( visibleBlocks ).toHaveLength( 0 );
Expand All @@ -114,7 +114,7 @@ describe( 'InserterMenu', () => {
position={ 'top center' }
instanceId={ 1 }
items={ [] }
recentItems={ [] }
frecentItems={ [] }
debouncedSpeak={ noop }
fetchReusableBlocks={ noop }
/>
Expand All @@ -124,13 +124,13 @@ describe( 'InserterMenu', () => {
expect( visibleBlocks ).toHaveLength( 0 );
} );

it( 'should show the recently used items in the recent tab', () => {
it( 'should show the frequently used items in the frequent tab', () => {
const wrapper = mount(
<InserterMenu
position={ 'top center' }
instanceId={ 1 }
items={ items }
recentItems={ [ advancedTextItem, textItem, someOtherItem ] }
frecentItems={ [ advancedTextItem, textItem, someOtherItem ] }
debouncedSpeak={ noop }
fetchReusableBlocks={ noop }
/>
Expand All @@ -149,7 +149,7 @@ describe( 'InserterMenu', () => {
position={ 'top center' }
instanceId={ 1 }
items={ items }
recentItems={ [] }
frecentItems={ [] }
debouncedSpeak={ noop }
fetchReusableBlocks={ noop }
/>
Expand All @@ -173,7 +173,7 @@ describe( 'InserterMenu', () => {
position={ 'top center' }
instanceId={ 1 }
items={ items }
recentItems={ [] }
frecentItems={ [] }
debouncedSpeak={ noop }
fetchReusableBlocks={ noop }
/>
Expand All @@ -196,7 +196,7 @@ describe( 'InserterMenu', () => {
position={ 'top center' }
instanceId={ 1 }
items={ items }
recentItems={ [] }
frecentItems={ [] }
debouncedSpeak={ noop }
fetchReusableBlocks={ noop }
/>
Expand All @@ -222,7 +222,7 @@ describe( 'InserterMenu', () => {
position={ 'top center' }
instanceId={ 1 }
items={ items }
recentItems={ items }
frecentItems={ items }
debouncedSpeak={ noop }
fetchReusableBlocks={ noop }
/>
Expand All @@ -239,7 +239,7 @@ describe( 'InserterMenu', () => {
position={ 'top center' }
instanceId={ 1 }
items={ items }
recentItems={ [] }
frecentItems={ [] }
debouncedSpeak={ noop }
fetchReusableBlocks={ noop }
/>
Expand All @@ -262,7 +262,7 @@ describe( 'InserterMenu', () => {
position={ 'top center' }
instanceId={ 1 }
items={ items }
recentItems={ [] }
frecentItems={ [] }
debouncedSpeak={ noop }
fetchReusableBlocks={ noop }
/>
Expand Down
1 change: 0 additions & 1 deletion editor/store/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
export const PREFERENCES_DEFAULTS = {
recentInserts: [],
insertUsage: {},
};
8 changes: 1 addition & 7 deletions editor/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,17 +656,12 @@ export function preferences( state = PREFERENCES_DEFAULTS, action ) {
id += '/' + block.attributes.ref;
}

const isSameAsInsert = ( { name, ref } ) => name === insert.name && ref === insert.ref;

return {
...prevState,
recentInserts: [
insert,
...reject( prevState.recentInserts, isSameAsInsert ),
],
insertUsage: {
...prevState.insertUsage,
[ id ]: {
time: Date.now(),
Copy link
Member

@aduth aduth Mar 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Pedantry: Date.now makes this reducer impure:

https://en.wikipedia.org/wiki/Functional_programming#Pure_functions

If a pure function is called with arguments that cause no side-effects, the result is constant with respect to that argument list ... i.e., if calling the pure function again with the same arguments returns the same result.

Violating a core principle of Redux:

Reducers are just pure functions that take the previous state and an action, and return the next state.

https://redux.js.org/introduction/three-principles#changes-are-made-with-pure-functions

Copy link
Member Author

Choose a reason for hiding this comment

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

You're totally right. I'll move this to the action creator.

count: prevState.insertUsage[ id ] ? prevState.insertUsage[ id ].count + 1 : 1,
insert,
},
Expand All @@ -678,7 +673,6 @@ export function preferences( state = PREFERENCES_DEFAULTS, action ) {
return {
...state,
insertUsage: omitBy( state.insertUsage, ( { insert } ) => insert.ref === action.id ),
recentInserts: reject( state.recentInserts, insert => insert.ref === action.id ),
};
}

Expand Down
35 changes: 20 additions & 15 deletions editor/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1186,30 +1186,35 @@ function getItemsFromInserts( state, inserts, enabledBlockTypes = true, maximum
}

/**
* Determines the items that appear in the 'Recent' tab of the inserter.
* Returns a list of items which the user is likely to want to insert. These
* are ordered by 'frecency', which is a heuristic that combines block usage
* frequency and recency.
*
* @param {Object} state Global application state.
* @param {string[]|boolean} enabledBlockTypes Enabled block types, or true/false to enable/disable all types.
* @param {number} maximum Number of items to return.
*
* @return {Editor.InserterItem[]} Items that appear in the 'Recent' tab.
*/
export function getRecentInserterItems( state, enabledBlockTypes = true, maximum = MAX_RECENT_BLOCKS ) {
return getItemsFromInserts( state, state.preferences.recentInserts, enabledBlockTypes, maximum );
}

/**
* Determines the items that appear in the inserter with shortcuts based on the block usage
* https://en.wikipedia.org/wiki/Frecency
*
* @param {Object} state Global application state.
* @param {string[]|boolean} enabledBlockTypes Enabled block types, or true/false to enable/disable all types.
* @param {number} maximum Number of items to return.
*
* @return {Editor.InserterItem[]} Items that appear in the 'Recent' tab.
*/
export function getFrequentInserterItems( state, enabledBlockTypes = true, maximum = MAX_RECENT_BLOCKS ) {
export function getFrecentInserterItems( state, enabledBlockTypes = true, maximum = MAX_RECENT_BLOCKS ) {
const calculateFrecency = ( time, count ) => {
const duration = Date.now() - time;
switch ( true ) {
case duration < 3600:
return count * 4;
case duration < ( 24 * 3600 ):
return count * 2;
case duration < ( 7 * 24 * 3600 ):
return count / 2;
default:
return count / 4;
}
};

const sortedInserts = values( state.preferences.insertUsage )
.sort( ( a, b ) => b.count - a.count )
.sort( ( a, b ) => calculateFrecency( b.time, b.count ) - calculateFrecency( a.time, a.count ) )
.map( ( { insert } ) => insert );
return getItemsFromInserts( state, sortedInserts, enabledBlockTypes, maximum );
}
Expand Down
25 changes: 6 additions & 19 deletions editor/store/test/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1196,13 +1196,12 @@ describe( 'state', () => {
const state = preferences( undefined, {} );

expect( state ).toEqual( {
recentInserts: [],
insertUsage: {},
} );
} );

it( 'should record recently used blocks', () => {
const state = preferences( deepFreeze( { recentInserts: [], insertUsage: {} } ), {
const state = preferences( deepFreeze( { insertUsage: {} } ), {
type: 'INSERT_BLOCKS',
blocks: [ {
uid: 'bacon',
Expand All @@ -1211,21 +1210,19 @@ describe( 'state', () => {
} );

expect( state ).toEqual( {
recentInserts: [
{ name: 'core-embed/twitter' },
],
insertUsage: {
'core-embed/twitter': {
time: expect.any( Number ),
Copy link
Member

Choose a reason for hiding this comment

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

To the above point, we shouldn't have any need for expect.any in this file, given that a reducer should be deterministic.

count: 1,
insert: { name: 'core-embed/twitter' },
},
},
} );

const twoRecentBlocks = preferences( deepFreeze( {
recentInserts: [],
insertUsage: {
'core-embed/twitter': {
time: expect.any( Number ),
count: 1,
insert: { name: 'core-embed/twitter' },
},
Expand All @@ -1243,16 +1240,14 @@ describe( 'state', () => {
} );

expect( twoRecentBlocks ).toEqual( {
recentInserts: [
{ name: 'core/block', ref: 123 },
{ name: 'core-embed/twitter' },
],
insertUsage: {
'core-embed/twitter': {
time: expect.any( Number ),
count: 2,
insert: { name: 'core-embed/twitter' },
},
'core/block/123': {
time: expect.any( Number ),
count: 1,
insert: { name: 'core/block', ref: 123 },
},
Expand All @@ -1262,13 +1257,9 @@ describe( 'state', () => {

it( 'should remove recorded reusable blocks that are deleted', () => {
const initialState = {
recentInserts: [
{ name: 'core-embed/twitter' },
{ name: 'core/block', ref: 123 },
{ name: 'core/block', ref: 456 },
],
insertUsage: {
'core/block/123': {
time: 1000,
count: 1,
insert: { name: 'core/block', ref: 123 },
},
Expand All @@ -1281,10 +1272,6 @@ describe( 'state', () => {
} );

expect( state ).toEqual( {
recentInserts: [
{ name: 'core-embed/twitter' },
{ name: 'core/block', ref: 456 },
],
insertUsage: {},
} );
} );
Expand Down
Loading